-
Notifications
You must be signed in to change notification settings - Fork 0
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] Removed local_node_query
profile
#72
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rmanaem for the PR!
A few comments from me:
- Could you update the comment for
NB_API_QUERY_URL
intemplate.env
to specify that this should be the f-API URL, and ensure we use port 8080 in the example to avoid confusion? - Similarly, I think we should update the description for
NB_FEDERATE_REMOTE_PUBLIC_NODES
inneurobagel_environment_variables.tsv
to mention that when this is false, only locally specified nodes (inlocal_nb_nodes.json
) are queried (or sth to that effect) - The links in the ## How to use section of the README are out of date. Can you update this to point to the Getting started / Configuring a node sections of the docs?
- I think it might also be nice to link to the "Configuring a node" page at the top of the environment variable TSV, so users can find more info about the env vars - will leave that up to you
- If it makes sense to you, let's wait until https://github.com/neurobagel/recipes/issues/71 is merged before merging this PR so that this change to the profiles can be versioned
Not sure what you had in mind here, do you mean to add another row in the tsv? |
Whoops, sorry that was a typo from my end. I meant in the template.env file, not the environment variable TSV! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one 🍒, otherwise 🧑🍳 !
Co-authored-by: Alyssa Dai <[email protected]>
Blocked until #75 is addressed |
This is ready to be merged! |
🚀 PR was released in |
local_node_query
profile from docker-compose.yml #70Changes proposed in this pull request:
local_node_query
profileNB_IS_FEDERATION_API
env varChecklist
This section is for the PR reviewer
[ENH]
,[FIX]
,[REF]
,[TST]
,[CI]
,[MNT]
,[INF]
,[MODEL]
,[DOC]
) (see our Contributing Guidelines for more info)Closes #XXXX
For new features:
For bug fixes: