Skip to content
This repository has been archived by the owner on Jul 3, 2023. It is now read-only.

Adding container image for running examples #203

Closed
wants to merge 1 commit into from
Closed

Adding container image for running examples #203

wants to merge 1 commit into from

Conversation

bovem
Copy link
Contributor

@bovem bovem commented Oct 6, 2022 •
edited
Loading

[Short description explaining the high-level reason for the pull request]

Changes

  • Added a dockerfile for the image that could be used for running examples.
  • Each example folder has a python virtual environment named hamilton that should be activated first before running the example.

Testing

  • Build the container image
docker build --tag hamilton-example .
  • Creating a container
docker run -it --rm --name hamilton-example hamilton-example
  • Running the hello_world example inside the container
cd hamilton/example/hello_world
source hamilton-env/bin/activate
python my_script.py

Notes

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.
  • Reviewers requested with the Reviewers tool ➡️

Testing checklist

Python - local testing

  • python 3.6
  • python 3.7
  • python 3.9 (Docker Environment)

@bovem bovem mentioned this pull request Oct 6, 2022
Closed
@bovem bovem marked this pull request as draft October 6, 2022 05:22
@bovem bovem marked this pull request as ready for review October 10, 2022 05:40
skrawcz
skrawcz reviewed Oct 10, 2022
View reviewed changes
examples/README.md Outdated
## Running examples through docker image
- Build the container image
```bash
docker build --tag hamilton-example .
Copy link
Collaborator

@skrawcz skrawcz Oct 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

what directory should people be in? I know this is under examples/ but just to be explicit.

Copy link
Collaborator

@skrawcz skrawcz Oct 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

also mention how long the build process might take.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added build time and directory details.

skrawcz
skrawcz reviewed Oct 10, 2022
View reviewed changes
examples/README.md Outdated
docker run -it --rm --name hamilton-example hamilton-example
```

- Running the `hello_world` example inside the container
Copy link
Collaborator

@skrawcz skrawcz Oct 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

can you add the instruction for starting the container if you've already built it please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added note to skip (to step 3 container creation) if image is already present.

skrawcz
skrawcz reviewed Oct 10, 2022
View reviewed changes
examples/make_python_virtualenv.sh
@@ -0,0 +1,28 @@
# Get a list of all the folders containing "requirements.txt" file
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a comment at the top as to the purpose of this file please?
E.g. "Use this to create individual virtual environments for each example - it assume virtualenv has been installed"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added description of the script at the top. Along with its usage

skrawcz
skrawcz reviewed Oct 10, 2022
View reviewed changes
examples/README.md
@@ -21,3 +21,23 @@ folders.
Under `model_examples` you'll find a how you could apply Hamilton to model your ML workflow.
Check it out to get a sense for how Hamilton could make your ML pipelines reusable/general
components...

## Running examples through docker image
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add more to say how this works please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added documentation for docker image

@elijahbenizzy
Copy link
Collaborator

Saw @skrawcz took a look. Two more things:

  1. Make sure to install./run pre-commit checks on everything (the pre-commit is failing). See

    hamilton/developer_setup.md

    Line 16 in 5111491

    4. `brew install pre-commit` if you haven't.
    . You may want to reset to the merging branch then pack it all into a single commit and let that run the pre-commit for you.
  2. Add yourself to the contributor list!

@bovem bovem closed this Oct 13, 2022
@skrawcz
Copy link
Collaborator

skrawcz commented Oct 13, 2022

@bovem let me know when this PR is back up and ready -- it's on my list to pull down and verify things myself. Thanks :)

@bovem
Copy link
Contributor Author

bovem commented Oct 13, 2022

Hi @skrawcz I forced pushed some commits by mistake and it closed this this PR. I have created this PR: #209 from a new branch with pre-commit checks.
I am sorry if it caused any inconvenience.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@skrawcz skrawcz skrawcz left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bovem @elijahbenizzy @skrawcz