search

man-group / notebooker

Productionise & schedule your Jupyter Notebooks as easily as you wrote them.
GNU Affero General Public License v3.0
853 stars 80 forks source link

Test isolation #20

Closed Code0x58 closed 3 years ago

Code0x58 commented 3 years ago

This a commit on top of #14

In removing the defaults from the subprocess (which look like they should come from the parent process and it's method of getting defaults), a fair few tests broke, and it looks like tests relied on side-effects of others or those defaults. The change touched a fair chunk of tests as a result.

Notes on notebooker.execute_notebook.main

It looks like it could be good to refactor the code to avoid the use of environment variables down in the code (e.g. get_cache_dir()), and to instead only have it on the entrypoints - this should help isolation a bit. Click supports it for options [docs], and there are also there are things like pydantic.BaseSettings which can be used.

Given that the settings are used in two places (the server, and the child processes), I think something like pydantic.BaseSettings would be DRYer as you have one magically instantiating class to gather and sanitise all of the variables, rather than duplicating CLI options on the two entrypoints.

This would also move away from the having the config baked into the codebase which is switched using an environment variable, so you'd have less indirection, and also separate code from deployment.

jonbannister commented 3 years ago

Yep I like the idea of uniting the parsing of the common entrypoints and a global config object using pydantic or similar. We just need to ensure it is consistent between parent/child processes.