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

Bump pyyaml 6.0 -> 6.0.1 and Pillow 9.0.1 -> 10.3.0 to avoid local install issues #81

Merged
merged 2 commits into from
Apr 20, 2024

Conversation

mcoot
Copy link
Collaborator

@mcoot mcoot commented Apr 15, 2024

Tiny, tiny PR just because I couldn't get requirements installed on my machine due to:

For PyYaml it's a patch version bump so should be safe. And Pillow is only even here as a transitive dependency of scipy which is used in the script for plotting Trueskill, it shouldn't be actually required to run the app.

@StephenBartos
Copy link
Collaborator

StephenBartos commented Apr 19, 2024

And Pillow is only even here as a transitive dependency of scipy which is used in the script for plotting Trueskill, it shouldn't be actually required to run the app.

I'm not too sure how to handle dependencies that are required for the scripts to run. Would providing a nested requirements.txt and mandating a new .venv be created be a possible solution? I'm not an expert on package managing in python for situations like this. I do agree that requiring dependencies for the scripts shouldn't be included in the main requirements.txt.

Copy link
Collaborator

@StephenBartos StephenBartos left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup of the docs and finding this dependency. Let me know if you have any solutions for how we should manage the dependencies of the scripts Vs. the dependencies necessary to run the bot. Either way, these changes are great.

@Andrewki44
Copy link
Collaborator

Does the docker image need to be updated to include the updated packages?

@mcoot
Copy link
Collaborator Author

mcoot commented Apr 20, 2024

Does the docker image need to be updated to include the updated packages?

As long as the base image is being built inline and not pulled from dockerhub (which is the case in the dockerfile as it stands), it shouldn't.

@Andrewki44
Copy link
Collaborator

Andrewki44 commented Apr 20, 2024

Understood, thanks for clarifying.
Merging

@Andrewki44 Andrewki44 merged commit 1df4e5a into tribesonecommunity:master Apr 20, 2024
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.

3 participants