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

NF: Add Environment class, with initial Native/Docker implementations #516

Merged
merged 39 commits into from
Jan 29, 2024

Conversation

effigies
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Mar 22, 2022

Codecov Report

Attention: 95 lines in your changes are missing coverage. Please review.

Comparison is base (e425b83) 83.42% compared to head (f3c1d35) 81.53%.
Report is 62 commits behind head on master.

❗ Current head f3c1d35 differs from pull request most recent head 0b0c71b. Consider uploading reports for the commit 0b0c71b to get more accurate results

Files Patch % Lines
pydra/engine/workers.py 25.35% 53 Missing ⚠️
pydra/engine/environments.py 70.76% 19 Missing ⚠️
pydra/engine/run_pickled.py 23.80% 16 Missing ⚠️
pydra/conftest.py 60.00% 6 Missing ⚠️
pydra/engine/task.py 97.29% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #516      +/-   ##
==========================================
- Coverage   83.42%   81.53%   -1.90%     
==========================================
  Files          22       24       +2     
  Lines        4894     4949      +55     
  Branches     1410        0    -1410     
==========================================
- Hits         4083     4035      -48     
- Misses        807      914     +107     
+ Partials        4        0       -4     
Flag Coverage Δ
unittests 81.53% <57.58%> (-1.90%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@djarecka
Copy link
Collaborator

for now test_docker.py and test_singularity.py might not work, examples for docker env are in test_environments.py
(still more needed)

@ghisvail
Copy link
Collaborator

I was able to test this PR successfully on pydra-deface using FSL tasks and a container image built from the latest version of FSL.

A few paths of improvement:

  • Although docker is the most "popular", podman is also a suitable alternative. It would be nice to offer the possibility to use any of those somehow. I was able to test podman by directly editing the source code in my test venv.

  • On the same vain, the Docker registry is certainly the most popular because it is the default, but it would also be worth being able to specify an alternative registry such as Quay or GHCR for instance.

More info can be found in the containers transport documentation.

@effigies
Copy link
Contributor Author

I agree, we should definitely try to keep this general. I don't have enough experience with non-Docker/non-Singularity runtimes to be opinionated here, so happy to follow your lead.

@ghisvail
Copy link
Collaborator

I did a bit of research:

  • Regarding supporting podman in addition to docker, since the former provides a compatible CLI with the latter, Linux distributions either advise using an alias or provide a compatibility package. So it sounds fine to assume the runtime will have a docker command available at first, before thinking of a means to override it if need be.

  • Regarding support for other registries, I think the most comfortable is to leave it as part of the image name, i.e. ghcr.io/$username/$image, instead of introducing a registry parameter. Let docker or podman deal with the default registry
    based on their respective settings.

To conclude, I think the current implementation is good enough as it is 👍

@effigies effigies force-pushed the rf/environments branch 2 times, most recently from fb9dddd to 48b35b7 Compare August 29, 2023 12:07
@effigies
Copy link
Contributor Author

effigies commented Aug 29, 2023

@djarecka This is passing locally, but there are a lot of skipped tests. We'll see how far this gets on CI, but this is it from me for today.

@effigies effigies marked this pull request as ready for review August 30, 2023 17:59
@djarecka
Copy link
Collaborator

djarecka commented Sep 5, 2023

@effigies - ok, thank you. I will review the tests that are failing

Copy link
Contributor Author

@effigies effigies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked at this with some fresh eyes and left a few notes. We should be careful to name the functions and attributes to make future contributors (such as ourselves) less confused.

pydra/engine/environments.py Show resolved Hide resolved
pydra/engine/environments.py Outdated Show resolved Hide resolved
pydra/engine/task.py Outdated Show resolved Hide resolved
pydra/engine/task.py Outdated Show resolved Hide resolved
for fld in attr_fields(self.inputs):
if TypeParser.contains_type(FileSet, fld.type):
# Is container_path necessary? Container paths should just be typed PurePath
assert not fld.metadata.get("container_path")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is presumably failing in test_docker_inputspec_3. I think we need a clearer use-case and figure out the correct behavior. In any case, we've been putting up with a failing test for a while. I'm inclined to say let's re-add this feature when someone actually needs it.

pydra/engine/task.py Outdated Show resolved Hide resolved
pydra/engine/task.py Outdated Show resolved Hide resolved
pydra/engine/task.py Outdated Show resolved Hide resolved
pydra/engine/task.py Outdated Show resolved Hide resolved
@djarecka
Copy link
Collaborator

djarecka commented Sep 7, 2023

i'm trying to understand why the slurm test fails now, since you didn't any new test here yet. any idea?

@effigies
Copy link
Contributor Author

effigies commented Sep 7, 2023

Nope. I've never understood anything about the slurm tests. I'll rebase on master and push, if that might help.

Copy link
Collaborator

@ghisvail ghisvail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the value proposition from the proposed Environment abstraction is clear, I am just a bit confused about the execution.

It sounds to me like they could be simplified to a bunch of context managers, maybe:

# somewhere in the submitter's code
match environment:  # assuming [type, *args]
    case ["docker", image, tag]:
        with setup_docker(task, image, tag) as env:
            ...
    case _:  # fallback to native
        with setup_native(task) as env:
            ...

Here I am using the new pattern matching syntax, but a stack of isintance calls would work the same.

The other worry I have got is how composable the Environment family of values is with the Submitter one. All submitters (cf, Dask, SLURM, ...) should work with native, but would that be the case for containers (Docker, Singularity) or others we had in mind like Conda?

I am totally onboard with the idea to dissociate the execution environment from the task abstraction. But I am still feeling the boundary between Environment and Submitter is not as clear cut than anticipated? I don't have further ideas to improve the proposal at this point, so I won't be opposed to a merge 👍

pydra/engine/environments.py Show resolved Hide resolved
pydra/engine/specs.py Outdated Show resolved Hide resolved
@djarecka djarecka mentioned this pull request Nov 3, 2023
2 tasks
Copy link
Contributor Author

@effigies effigies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty much ready to go. Small comments.

pydra/engine/core.py Outdated Show resolved Hide resolved
pydra/engine/environments.py Outdated Show resolved Hide resolved
pydra/engine/environments.py Outdated Show resolved Hide resolved
pydra/engine/environments.py Show resolved Hide resolved
pydra/engine/environments.py Outdated Show resolved Hide resolved
pydra/engine/environments.py Outdated Show resolved Hide resolved
pydra/engine/tests/test_specs.py Outdated Show resolved Hide resolved
@djarecka
Copy link
Collaborator

@ghisvail @tclose - want to try this before merging?

@tclose
Copy link
Contributor

tclose commented Nov 21, 2023

@ghisvail @tclose - want to try this before merging?

Don't have the time atm, so happy to go with this and find problems later

@effigies effigies merged commit dc121df into master Jan 29, 2024
37 of 38 checks passed
@effigies effigies deleted the rf/environments branch January 29, 2024 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants