-
Notifications
You must be signed in to change notification settings - Fork 223
Implement wave container commands #3954
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
base: dev
Are you sure you want to change the base?
Implement wave container commands #3954
Conversation
…anFlesch/nf-core-tools into feature/3952-create-waveyml
…t instead of printing. simplify checks
…/nf-core-tools into feature/3952-create-waveyml
…/nf-core-tools into feature/3952-create-waveyml
nf_core/modules/containers.py
Outdated
| ] | ||
| return containers_flat | ||
|
|
||
| def _resolve_module_dir(self, module: str | Path) -> Path: |
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.
do we not have this funciton already somehwere else?
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.
I might be overlooking something, but not really ... As far as I can tell, we have ModulesInfo, which was used at some point, but it never provides the environment.yml or meta.yml paths, or confirms their existance.
Maybe this should be in ModulesInfo?
nf_core/modules/containers.py
Outdated
| return module_dir | ||
|
|
||
| @staticmethod | ||
| def _environment_path(module_dir: Path) -> Path: |
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.
because we lint for this, I think we should be able to reuse something there.
|
|
||
|
|
||
| @contextmanager | ||
| def set_wd_tempdir() -> Generator[None, None, None]: |
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.
this function is for sure created somewhere else already.
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.
Yes, In pipeline downloads! But that version throws Download-specific errors ...
The plan was move it out of the utils.py over there into the general utils.
But now we don't use it at all anymore ...
Should we remove it again?
| IMAGE_KEY = "name" | ||
| BUILD_ID_KEY = "buildId" | ||
| SCAN_ID_KEY = "scanId" |
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.
I don't think any of these will change in the future. clearer if we just use them as strings in the code I think.
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.
If you don't feel strongly about this, I think we should keep them. They only exist in the class scope, close to where they are being used, but there we use them quite a lot as it helps us to not have redundant literals
nf_core/modules/containers.py
Outdated
| for platform in CONTAINER_PLATFORMS: | ||
| containers[cs][platform] = self.request_container(cs, platform, env_path, await_, dry_run) | ||
|
|
||
| for platform in CONTAINER_PLATFORMS: |
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.
do we need to loop twice over container_platforms?
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.
I thought it makes it more readable what's being done with being less nested and clearly separated. But for comparing, applied your suggestion in af97d90
Fix typo Co-authored-by: Matthias Hörtenhuber <[email protected]>
|
Closes #3952
PR checklist
CHANGELOG.mdis updateddocsis updated