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

Reduce the number of configurable options / environment variables to declutter or at least restructure #30

Closed
3 tasks done
surchs opened this issue Apr 5, 2024 · 5 comments · Fixed by #80
Closed
3 tasks done
Assignees
Labels
documentation Changes only affect the documentation flag:discuss Flag issue that needs to be discussed before it can be implemented. refactor Simplifying or restructuring existing code or documentation. released This issue/pull request has been released.

Comments

@surchs
Copy link
Contributor

surchs commented Apr 5, 2024

Now that we have a single .env file and a single docker-compose.yml file in the works, it is a bit easier to go through all the ENV variables we support / put in our template files. And there are quite a lot!

The issue here is that this can be overwhelming for a user to read through, e.g. in the setup instructions our ENV table is a big wall of text: https://neurobagel.org/infrastructure/#set-the-environment-variables.

I have these assumptions:

  • most deployments will have their own graph instance. This is another way of saying: most places will have just one node. But even for multiple nodes, it is a lot easier to spin up a new graph instance than to reuse an existing node instance and coordinate everything else to reuse it. The fact that the free graphDB tier only has a single thread only further emphasizes this
  • if the former is true, then there is no point in changing most of the graphDB setup config (DB name, user name, ...)
  • there are no good use cases to change the port a service is running on inside a container - other than "I like to be able to change everything because ..."
  • in the balance of "we serve every use case" and "we have brief, clear docs", it's better to lean towards brief, clear docs. Not everything that can be configurable should be configurable

With this in mind, I took a look at our variables, and I think there are three groups:

  • a small set of variables that really should / have to be set by the user in most cases
  • a much bigger group of variables that are sometimes useful, but in most cases should be ignored
  • a third group of variables that serve no good use case and should be removed

Here is my view:

Variable Required Sometimes Remove Notes
Graph
NB_GRAPH_ADMIN_PASSWORD
NB_GRAPH_USERNAME If we have a 1 node = 1 DB = 1 graph instance deployment, this is essentially irrelevant
NB_GRAPH_ADDRESS Don't know when we would change this
NB_GRAPH_PASSWORD
NB_GRAPH_DB same as username - no need to rename this if there is only 1 DB
NB_GRAPH_PORT_HOST situational, should eventually disappear completely once we isolate the graph from host completely as we said we would - this is needed to access the workbench
NB_GRAPH_PORT remove! again: can't think of any good use case where the user would have to change the port the graph is running on inside the container. rather everything gets a lot easier if this can not be changed - because all other places can just use the default value
NB_GRAPH_IMG remove! We only have one backend now, and I doubt we'll ever have 2 again, even if we switch away from graphDB
LOCAL_GRAPH_DATA This is a new one I just made up - to let user define where the local .jsonld files live - can be useful if that path is not a subdir of where the docker-compose.yml lives
n-API
NB_RETURN_AGG
NB_NAPI_TAG situational, e.g. for dev or rollback. I think we should suggest to not override latest in most cases
NB_NAPI_ALLOWED_ORIGINS required in most cases
NB_NAPI_PORT_HOST situational, helps deconflict ports. in federation deployment we could consider completely removing the ports / isolating the node from host
NB_NAPI_PORT remove! I can't think of a good reason for a user to configure the port inside the container
Query
NB_QUERY_TAG situational, but should discourage unless good reason to not use latest
NB_QUERY_PORT_HOST
NB_API_QUERY_URL Should we change the name to just NB_API_URL?
NB_QUERY_APP_BASE_PATH
f-API
NB_FAPI_PORT_HOST situational, to deconflict ports
NB_FAPI_PORT remove: port inside container - can't see a good reason to set this to anything other than default!
NB_FAPI_TAG same as others
NB_FEDERATE_REMOTE_PUBLIC_NODES
auth
NB_ENABLE_AUTH
NB_QUERY_CLIENT_ID

Based on this, my suggestion is:

  • Remove every container-internal port configuration
  • Remove most graph configuration options (e.g. the image name)
  • [ ] Restructure the template .env file so that the required variables sit at the top (with a comment explaining that they are needed), and the optional variables that are left sit in a separate section below
  • Trim the ENV variable table in the docs down to the required variables (the remaining optional ones can be in an expandable box)

I think our deployment docs are going to become radically shorter, once we adopt the single docker-compose file (#32), and decluttering the .env file will go a long way to making them even clearer!

Decision: we will do this first, only for the dev deployment docker compose recipe - and once we like it, give it to the default deployment for users.

@surchs surchs added documentation Changes only affect the documentation refactor Simplifying or restructuring existing code or documentation. type:maintenance labels Apr 5, 2024
@surchs surchs added this to Neurobagel Apr 5, 2024
@surchs surchs added the flag:discuss Flag issue that needs to be discussed before it can be implemented. label Apr 5, 2024
@alyssadai alyssadai moved this to Backlog in Neurobagel Apr 5, 2024
@rmanaem rmanaem moved this from Backlog to Specify - Done in Neurobagel Apr 5, 2024
@surchs surchs moved this from Specify - Done to Implement - Track in Neurobagel Apr 16, 2024
@surchs
Copy link
Contributor Author

surchs commented Apr 16, 2024

Blocked until we fix the problems with the docker compose recipe

@rmanaem
Copy link
Contributor

rmanaem commented Apr 29, 2024

The issue is now unblocked.

@rmanaem rmanaem added the someday Not a priority right now, but we want to keep this around to think or discuss more. label Apr 29, 2024
@rmanaem rmanaem removed the status in Neurobagel Apr 29, 2024
@alyssadai
Copy link
Contributor

As part of this issue, we might also want to revisit where default values for env variables should be set. At the moment, our docker-compose.yml recipe has fallback values for each env var that's used, using the ${MYVAR:-defaultvalue} syntax. However each repo's code (n-API, query tool, etc.) also specifies a default value if a needed environment variable is not found to be set, e.g., https://github.com/neurobagel/api/blob/2e70ffcc73577442ceb990a773be9c8444e8c252/app/api/utility.py#L32.

Generally speaking, the defaults are consistent at the docker compose and individual tool level, but not always. We may want to get rid of defaults in one of these places to avoid redundancy

@alyssadai alyssadai added the flag:schedule Flag issue that should go on the roadmap or backlog. label Jul 29, 2024
@surchs surchs moved this to Backlog in Neurobagel Aug 2, 2024
@surchs surchs removed the flag:schedule Flag issue that should go on the roadmap or backlog. label Aug 2, 2024
@surchs surchs moved this from Backlog to Specify - Active in Neurobagel Aug 13, 2024
@alyssadai
Copy link
Contributor

alyssadai commented Aug 15, 2024

Decisions:

  • Look into creating an extended docker-compose.yml that will allow us to stop exposing the graph port to the host
  • Consider renaming local_node and local_federation profiles

@alyssadai alyssadai moved this from Specify - Active to Implement - Active in Neurobagel Aug 15, 2024
@alyssadai alyssadai self-assigned this Aug 15, 2024
@alyssadai alyssadai removed the someday Not a priority right now, but we want to keep this around to think or discuss more. label Aug 15, 2024
@alyssadai alyssadai moved this from Implement - Active to Implement - Done in Neurobagel Aug 16, 2024
@github-project-automation github-project-automation bot moved this from Implement - Done to Review - Done in Neurobagel Aug 16, 2024
Copy link
Contributor

🚀 Issue was released in v0.2.0 🚀

@neurobagel-bot neurobagel-bot bot added the released This issue/pull request has been released. label Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Changes only affect the documentation flag:discuss Flag issue that needs to be discussed before it can be implemented. refactor Simplifying or restructuring existing code or documentation. released This issue/pull request has been released.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants