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

allow to mount host volumes in AWS Batch #640

Merged
merged 1 commit into from
Aug 20, 2021

Conversation

oavdeev
Copy link
Collaborator

@oavdeev oavdeev commented Aug 11, 2021

What does this PR do?

Adds an option to @batch decorator to mount host volumes inside the container. @batch(mount_host_volumes="/mnt/"). This way, /mnt from host will be mounted at /mnt inside the container. This (partially) addresses #441

In this release this is considered an advanced feature due to less friendly UX and the fact that troubleshooting this requires a decent knowledge of AWS Batch.

I've tested this on Batch and Step Functions and it seems to work!

Potential improvements in future PRs, not included here

  • First class support for per-job EFS mounts. Not included this because the configuration is pretty hairy, needs some extra thinking. However, you should be able to use this PR as is to expose EFS volumes mounted at host to containers, as described here.

  • Support for customizing mount points inside a container. Right now /mnt from host always gets mounted as /mnt inside the container.

@corleyma
Copy link

corleyma commented Aug 17, 2021

While a good start, I think this implementation falls short for a couple of reasons:

  • as mentioned, not being able to set the mount path inside the container seems like a strange limitation, and will likely place a burden on application developers to change their applications to fit the details of the infrastructure.
  • including this in @batch means it can only be used in flows that are forced to run a particular step on Batch (or else in every step if using with syntax)
    • it seems a more natural way for this to work would be some mechanism to specify these Batch-specific resource definitions in @resources (either as nested configs, or else as part of an @batch_resources decorator).
      • Combined with the ability to set the container mount path, this should make it easy to have local runs (e.g. for integration tests) that work seamlessly alongside remote runs in production without the need to fork application behavior.
      • This desire to define Batch-specific resources arises in other contexts, e.g. the desire to set a per-step job queue without forcing the step to run on Batch. Figuring out the idiom for this configuration in Metaflow and embracing it for all such configs would really increase the flexibility.

@oavdeev
Copy link
Collaborator Author

oavdeev commented Aug 17, 2021

Good points, tbh this PR was intended to solve a pretty narrow (advanced) use case of mounting EFS/FSx volumes from host into the container.

Re: local runs, wouldn't you have to fork the logic? You'd have to remember that the step may run in local mode and make sure to branch, something like:

@resources(batch_mount_points=["/mnt"])
def step_foo(self):
    if os.path.exists("/mnt"):
         # running on Batch
        model = load_model("/mnt/foo.tar.gz")
    else:
        # running locally
        model = MockLocalModel()
...

In this sense, one benefit of keeping it in @batch like in this PR that the UX is more foolproof; you're guaranteed that the step will run on Batch and /mnt will be there. I do see the benefit of being able to run the code locally without changing the annotation though.

@popsonebz
Copy link

popsonebz commented Aug 18, 2021

Thanks @oavdeev. I am hoping this PR gets merged as it worked as expected.
I was able to mount the efs by installing metaflow from your branch.

As you rightly said, adding the mount_host_volumes to one step did not affect the other steps.

Copy link
Collaborator

@savingoyal savingoyal left a comment

Choose a reason for hiding this comment

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

PR LGTM! Can we replace mount_host_volumes with host_volumes before shipping it?

@savingoyal savingoyal merged commit 0d052e8 into Netflix:master Aug 20, 2021
@oavdeev oavdeev deleted the batch-mount-host-volumes branch December 16, 2021 20:51
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