Make registry configurable #132
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@agitter This wound up being a bit more like open heart surgery than I anticipated. Let's discuss the approach and decide if we like doing things this way or not. |
agitter
reviewed
Sep 22, 2023
agitter
reviewed
Sep 22, 2023
jhiemstrawisc
force-pushed
the
make-registry-configurable
branch
from
41f9ec8 to
1313446
Compare
|
@agitter, I think I've covered all of the things we discussed in our last meeting, and this should more or less be ready for a final review. |
|
Merging #126 created merge conflicts in this pull request |
agitter
requested changes
Oct 13, 2023
jhiemstrawisc
force-pushed
the
make-registry-configurable
branch
from
8d2d076 to
7ecce69
Compare
The biggest change in this commit is the addition of `src/config.py`, which acts as a singleton module to pass global configs around SPRAS. When the Snakefile is run, it uses it's magically available `config` to instantiate the config object in the config module. After instantiation, other modules can `import src.config as config`, which allows them to access various config parameters. For example, to access the container framework, you can do: ``` import src.config as config framework = config.config.framework ``` In addition to modularizing configuration, I also added the ability to specify container registries and owners in the config. A full container name looks like `docker.io/reedcompbio/pathlinker:latest`. The config option considers docker.io the base_url, reedcompbio the owner, and then pathlinker will be hardcoded by that method. Lacking in this implementation is the ability to specify exactly what tag to run each PRM against. With the new config module, that should be relatively simple to add. As far as implentation choices, since the config module makes various values globally available, I decided to stop passing some values like `singularity` down through nested levels of function calls, and instead get those values close to where they're needed.
I updated `registry` to `container_registry` in anticipation of a future where we support different data registries as well, per @agitter's suggestion. I also changed the `config.framework` variable to `config.container_framework` to be more clear what this variable is supposed to contain.
This commit moves the container framework up a level so that each algorithm is called with the configured framework instead of determining the framework internally. The logic behind this is that doing so makes testing much easier; instead of messing with global configs in the tests, just call the PRM's run function with a different framework specified. This also moves prepare_volume to `containers.py` to fix the issue with importing hash lengths in the function. Arguably, I should have moved prepare_volume into containers.py in the first place.
jhiemstrawisc
force-pushed
the
make-registry-configurable
branch
from
b24ed46 to
b6f5f58
Compare
agitter
requested changes
Jan 5, 2024
agitter
approved these changes
Jan 14, 2024
This was referenced Jan 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add mechanism for passing global configurations
The biggest change in this commit is the addition of
src/config.py, which acts as asingleton module to pass global configs around SPRAS. When the Snakefile is run, it
uses its magically available
configto instantiate the config object in the configmodule. After instantiation, other modules can
import src.config as config, which allowsthem to access various config parameters. For example, to access the container framework,
you can do:
In addition to modularizing configuration, I also added the ability to specify container
registries and owners in the config. A full container name looks like
docker.io/reedcompbio/pathlinker:latest. The config option considers docker.io the base_url,reedcompbio the owner, and then pathlinker will be hardcoded by that method. Lacking in this
implementation is the ability to specify exactly what tag to run each PRM against. With the new
config module, that should be relatively simple to add.
As far as implentation choices, since the config module makes various values globally available,
I decided to stop passing some values like
singularitydown through nested levels of functioncalls, and instead get those values close to where they're needed. The only place this was obviously
not the clean way to do this was in the tests -- for now, I toggle
config.config.frameworkin theplaces where we need to test singularity.