Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENH] Adds initial workflow class #14

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

rmarkello
Copy link
Member

For running the primary map comparison workflows (with some light in-memory caching, as needed).

@rmarkello
Copy link
Member Author

Okay, I'm gonna be real: I have absolutely no clue why this is failing. These data are public on OSF and can be downloaded. I have manually tested this every which way and cannot figure it out. Calling in the big guns with @liuzhenqi77 @justinehansen: any thoughts on why this is failing ?

@liuzhenqi77
Copy link
Member

liuzhenqi77 commented Nov 9, 2021

Hiiii @rmarkello Running the tests locally also works fine for me. After some repeated trying using https://github.com/nektos/act, it seems changing session=requests.Session() at https://github.com/netneurolab/neuromaps/blob/main/neuromaps/datasets/annotations.py#L226-L227 solves the problem, although I'm not sure why reusing the session would lead to the error (when it worked totally fine locally 🤷‍♂️ ). This issue seems related (or not): python-poetry/poetry#3230.

I also added sudo apt-get update and sudo apt-get install -y libglu1-mesa in install_dependencies.sh when wb_command reported missing libGL.so.1 (this works but may not be the right lib to install).

@rmarkello
Copy link
Member Author

Amazing, @liuzhenqi77 !!! I didn't know the act library existed and that's awesome ! Thank you so much for pinning this suuuuper weird edge-case bug down.

I think I'd rather replace the relevant lines with repeat calls to _get_session(token=token) since we may need the token in certain instances—though I guess since all the annotations data are released now it might not be necessary 🤔 Stilll, no harm in doing that !

Re wb_command: I never know which of these libraries are right and basically trial-and-error it until it works, so if that worked for you then great !

I think I allowed maintainers to make changes to this branch so you could push the fixes if you want / are able since I won't be able to get to this for a little while...

Thank you again ! 🎉✨

rmarkello and others added 4 commits November 10, 2021 07:23
When reusing requests.Session in a container it seems there are
occasionally transient connectivity issues. Here we reinstantiate a
session for every file fetch
@liuzhenqi77
Copy link
Member

Now, requests.Session() works but whyyyy _get_session(token=token) not working?

@liuzhenqi77
Copy link
Member

Ohhh it seems requests is not the problem, but NEUROMAPS_OSF_TOKEN was not unset properly in another test, it was set to test_env. So I reverted the session changes to before and now it works.

Then there's another question: if NEUROMAPS_OSF_TOKEN is set correctly, will it fail the public annotations? If yes then we need to send those request without authorization header.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants