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

Python -- try to add some common packages to the ottr_python Dockerfile #30

Merged
merged 1 commit into from
Aug 15, 2024

Conversation

kweav
Copy link
Contributor

@kweav kweav commented Aug 14, 2024

Added a pip3 install command as described in the OTTR documentation. Specifically adding these packages per Chris's request: pandas, matplotlib, seaborn, numpy, plotnine, scikit-learn, & scipy. Currently no version is specified so latest versions are being installed. Will test and specify versions later once we've verified these work.

@kweav
Copy link
Contributor Author

kweav commented Aug 14, 2024

I tried to follow the OTTR documentation steps to test the dockerfile locally and it failed with a message of "denied: requested access to the resource is denied" once I was on the docker push step. It worked prior to that. I don't think I can use the Test build of Dockerfile GitHub Actions instructions because I don't see that workflow in the repo of interest. Please advise how I should test it further @cansavvy

@kweav kweav requested a review from cansavvy August 14, 2024 21:45
Copy link
Contributor

@cansavvy cansavvy left a comment

Choose a reason for hiding this comment

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

This builds correctly and seems reasonable.

  1. Let's merge this now
  2. Push to dockerhub with a tag like ottr_python:test using "Manual build" GitHub action
  3. Test that it makes Chris's test case work (change config_automation.yml docker designation as needed).
  4. Push the image again to dockerhub but with the main tag ottr_python:main using "Manual build" GitHub action

@kweav
Copy link
Contributor Author

kweav commented Aug 15, 2024

Great will try this! Thank you!

@kweav kweav merged commit a44cc26 into main Aug 15, 2024
6 checks passed
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