[ENH] Document using configure-nb for production deployment setup#396
[ENH] Document using configure-nb for production deployment setup#396
configure-nb for production deployment setup#396Conversation
✅ Deploy Preview for neurobagel-documentation ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
configure-nb for production deployment set up
configure-nb for production deployment set upconfigure-nb for production deployment setup
surchs
left a comment
There was a problem hiding this comment.
Thanks @alyssadai for the docs. Really like the simplicity of the .ini file. Makes it all much easier.
I think we should now get rid of some of the sections in the launch profiles because they get quite repetitive. Most of this info can be handled via annotations.
For node profile, I'd say:
definite annotations / remove sections:
- Set node deployment profile
- Set node domain
- Set node subdirectory path
could be annotations:
- Add data to the node (might be useful to link to)
- Set node response granularity (could be annotation with an extra admonition for context)
- Set graph store credentials (would remove / hide mainly because this isn't really security relevant / we should consider scrapping)
For portal profile:
definite annotations / remove sections:
- Set portal deployment profile
- Set portal domains
- Set portal subdirectory paths
could be annotation
- Set nodes to federate (mainly because of the number of admonitions. still think annotations would be cleaner)
With that, I think we'd have a very nice and compact prod-deployment recipe. And once the wizard has interactive-mode, we can remove most of the guide section for the prod-deployment and merge it with the current .env table to make it more of a detail reference.
| NB_GRAPH_DB=repositories/DB_NAME | ||
| NB_GRAPH_USERNAME=DB_USER |
There was a problem hiding this comment.
Are these really required variables? I.e. will the setup fail if these are not set?
There was a problem hiding this comment.
In the current deployment recipe (sans configure-nb), NB_GRAPH_USERNAME is required. With configure-nb, both are now optional since the tool sets default values for both variables.
I agree that we should revisit if we want to expose these variables/make them configurable by the user. I've opened an issue for us to discuss this:
The main consequence for users if we remove this instruction from the docs/rely on configure-nb-supplied defaults would be:
Existing users who have customized these variables in their .env file will need to reset their graph store, because configure-nb will overwrite the .env file with the default
NB_GRAPH_DBandNB_GRAPH_USERNAMEvalue
That said, I'm going to remove this section for now since I think this is a reasonable trade-off. Lmkwyt.
|
Thanks @surchs! To reduce confusion around the per-variable code snippets, I've switched to a format I've seen in the FastAPI docs: showing the full example INI in each section, but highlighting only the relevant lines. Per your suggestion, I've switched to an annotation for
Agreed! That said, until we have the separate guide/reference page, I think having only annotations for most variables would actually be less readable and more error-prone. For now, this page is still meant to be read in its entirety (at least for a given profile), whereas annotations feel more like "additional info"/are easier to miss - and have the added friction of requiring users to hover over each one. A lot of the guidance we currently have in the sections would also be hard to fit into a single annotation, and we'd lose the ability to link to specific sections. Even for vars like domain and subpath, I think this is valuable since these have tripped users up before (e.g. missing forward slashes), and these are required variables that must be set manually by the user (i.e., they cannot use the values in the example INI provided). I opened a separate issue for moving the guide-style content to another page, at which point I think annotations for the remaining variables in the example INI would make much more sense! |
configure-nbfor production deployments #371Changes proposed in this pull request:
configure-nb.envandlocal_nb_nodes.jsonwith instructions for using thenb_config.inifileNote
nb_config.inisections for variables. We should address this soon as part of Turn environment variables reference table into a glossary-style reference #337Checklist
Please leave checkboxes empty for PR reviewers
[ENH],[FIX],[REF],[TST],[CI],[MNT],[INF]) see our Contributing Guidelines for more info)Closes #XXXXmkdocs.ymlconfig