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

Support for Argo backend #19

Merged
merged 60 commits into from
Dec 7, 2022
Merged

Support for Argo backend #19

merged 60 commits into from
Dec 7, 2022

Conversation

Nilabhra
Copy link
Collaborator

@Nilabhra Nilabhra commented Nov 14, 2022

Additions:

  • Support for executing pipelines as Argo Workflows on Kubernetes.
  • Pipelines would be represented as DAGs, making pipeline representations generic.
  • .dockerignore file to filter out unneeded files from copied over to the container. Currently, everything in the project root get's copied over, including inputs and outputs.

Changes:

  • Modification of the dockerize module to support tagging + pushing a docker image to Docker Hub. For local deployments, later, support for a local docker registry needs to be added.
  • Modification of the example.py file to not accept the transaction model file name directly. The assumption now is that the user would provide the path to the directory where the pre-trained model binary is present. This had to be done as Kubernetes doesn't allow mounting of files as volumes. This modification seemed reasonable to me since most saved models use a unique directory name with a generic name for the weights binary (such as model.bin or weights.pt).
  • I was unable to get the shell scripts running as they were. I decided to just use python, which seems to work for me and @nikhil.
  • The dockerize module avoids setting the entry point to conda run -n pircli. While this works perfectly for docker-compose, it fails to work for Argo. I decided to just set the PATH env var to the location of the python binary for the pircli environment. That worked both for docker and for Argo.

Closes:

Comments:

I have tested out both the Argo backend the existing Docker backend and ensure they both work with the recent changes.

Commit history:

  • add: subparser for argoize

  • add: Nikhil's changes.

  • feat: generating workflow name using output file name.

  • feat: function to create Argo template from node.

  • chore: added docstring.

  • chore: added comments.

  • feat: setting the env vars before generation of the argo yaml.

  • feat: function to generate NFS volume specs for K8s.

  • feat: added support for mounting NFS volumes.

  • fix: using absolute path for the NFS mount paths.

  • fix: typo.

  • feat: function for generating Argo templates for PIRlib Graph objects.

  • chore: added comments.

  • feat: enabled creation of DAG tasks.

  • chore: added Argo specific refactoring.

  • chore: refactored argo task names.

  • fix: files cannot be mounted as volumes in Argo/K8s.

  • chore: removed redundant module.

  • chore: debugging code.

  • chore: optimized docker image generation.

  • add: dockerignore file to prevent copying of unnecessary files.

  • fix: missing volume mount.

  • chore: reverted to original formatting.

  • chore: removed spurious code.

  • chore: increased black linelength to 100.

Nilabhra and others added 30 commits November 10, 2022 10:47
* add: subparser for argoize

* add: Nikhil's changes.

* feat: generating workflow name using output file name.

* feat: function to create Argo template from node.

* chore: added docstring.

* chore: added comments.

* feat: setting the env vars before generation of the argo yaml.

* feat: function to generate NFS volume specs for K8s.

* feat: added support for mounting NFS volumes.

* fix: using absolute path for the NFS mount paths.

* fix: typo.

* feat: function for generating Argo templates for PIRlib Graph objects.

* chore: added comments.

* feat: enabled creation of DAG tasks.

* chore: added Argo specific refactoring.

* chore: refactored argo task names.

* fix: files cannot be mounted as volumes in Argo/K8s.

* chore: removed redundant module.

* chore: debugging code.

* chore: optimized docker image generation.

* add: dockerignore file to prevent copying of unnecessary files.

* fix: missing volume mount.

* chore: reverted to original formatting.

* chore: removed spurious code.

* chore: increased black linelength to 100.
@Nilabhra Nilabhra requested a review from zhanyuanucb November 28, 2022 05:52
@Nilabhra Nilabhra self-assigned this Nov 28, 2022
@zhanyuanucb
Copy link
Contributor

Can we include the requirements to run the new Argo example?

Also, we can include the generated YAML files from the new example, like docker and local backends

If this makes the example dir looks messy, we can organize different example in different folders.

pirlib/backends/argo_batch.py Outdated Show resolved Hide resolved
pirlib/backends/argo_batch.py Outdated Show resolved Hide resolved
pirlib/backends/argo_batch.py Outdated Show resolved Hide resolved
pirlib/cli/dockerize.py Outdated Show resolved Hide resolved
pirlib/cli/dockerize.py Outdated Show resolved Hide resolved

### Module 1: Docker_Packaging
python $ROOTDIR/bin/pircli dockerize \
--auto $ROOTDIR \
Copy link
Contributor

@zhanyuanucb zhanyuanucb Nov 29, 2022

Choose a reason for hiding this comment

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

TBH, —auto $ROOTDIR is a bit confusing.
May be something like this will be better?

$ROOTDIR \
--auto

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure!

@Nilabhra
Copy link
Collaborator Author

Nilabhra commented Nov 29, 2022

@zhanyuanucb

Can we include the requirements to run the new Argo example?

Sure, will add that.

Also, we can include the generated YAML files from the new example, like docker and local backends
If this makes the example dir looks messy, we can organize different example in different folders.

The issue with this is that it would expose specific information about my development setup such as:

  • My DockerHub name.
  • The name of the DockerHub repo.
  • The file locations location of the inputs/outputs e.g. /home/nilabhra/pirlib/example/inputs/train_dataset.

None of this is sensitive, so i have no issue exposing this if you are fine with it.

@Nilabhra Nilabhra requested a review from zhanyuanucb November 29, 2022 12:51
@zhanyuanucb
Copy link
Contributor

@Nilabhra
For those personal information, you can use something like xxx or @@@ to mask it out

@zhanyuanucb

Can we include the requirements to run the new Argo example?

Sure, will add that.

Also, we can include the generated YAML files from the new example, like docker and local backends
If this makes the example dir looks messy, we can organize different example in different folders.

The issue with this is that it would expose specific information about my development setup such as:

  • My DockerHub name.
  • The name of the DockerHub repo.
  • The file locations location of the inputs/outputs e.g. /home/nilabhra/pirlib/example/inputs/train_dataset.

None of this is sensitive, so i have no issue exposing this if you are fine with it.

@Nilabhra
Copy link
Collaborator Author

@zhanyuanucb

@Nilabhra
For those personal information, you can use something like xxx or @@@ to mask it out

Sure, just be aware that the YAML files would not work out of the box unlike the YAML file for docker-compose.

@Nilabhra
Copy link
Collaborator Author

Nilabhra commented Dec 5, 2022

@zhanyuanucb Up for another round of review.

@zhanyuanucb
Copy link
Contributor

@Nilabhra Could you put all the examples together in one .md file so that reader can see the toy example running on different backends? Maybe later the same .md file can be extended by the wiki_parser example, which is shown as an advance example.

The rest looks good to me.

@Nilabhra
Copy link
Collaborator Author

Nilabhra commented Dec 6, 2022

@zhanyuanucb

Could you put all the examples together in one .md file so that reader can see the toy example running on different backends? Maybe later the same .md file can be extended by the wiki_parser example, which is shown as an advance example.

I can copy-paste some of the content of the README.rst in the docs directory. Would that be fine? Or should I remove the Examples section from existing README.rst too?

@zhanyuanucb
Copy link
Contributor

@zhanyuanucb

Could you put all the examples together in one .md file so that reader can see the toy example running on different backends? Maybe later the same .md file can be extended by the wiki_parser example, which is shown as an advanced example.

I can copy-paste some of the content of the README.rst in the docs directory. Would that be fine? Or should I remove the Examples section from existing README.rst too?

@Nilabhra
Ah... my bad. I meant putting everything in the README.rst for now. Ideally, we can write documentation in .md and then use rst to organize the files, but this requires more work.

@zhanyuanucb zhanyuanucb merged commit 7786414 into petuum:master Dec 7, 2022
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.

2 participants