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

Remove version upper bound on pyyaml #2514

Merged
merged 3 commits into from
Sep 6, 2023

Conversation

steve-marmalade
Copy link
Contributor

@steve-marmalade steve-marmalade commented Sep 5, 2023

This PR removes the upper bound on pyyaml, thus allowing installation of PyYAML 6 and resolving the issues outlined in #2285. This is a blocking issue in order to use the GCP DeepLearning VM images (e.g. projects/deeplearning-platform-release/global/images/pytorch-1-13-cpu-v20230807-debian-11-py310), which come with PyYAML 6 already installed.

I manually patched my environment and was able to spin up a GCP VM and successfully run functions using runhouse.

I apologize but I don't have the required cloud accounts need to run the smoke-tests described below.

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (please specify below)
  • All smoke tests: pytest tests/test_smoke.py
  • Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
  • Backward compatibility tests: bash tests/backward_comaptibility_tests.sh

Copy link
Collaborator

@Michaelvll Michaelvll 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 submitting the PR @steve-marmalade! Just tested with aws in a new docker container and it seems working now as awscli has updated their dependency for pyyaml (aws/aws-cli@1b85174):

docker run -it --rm --name skypilot-debug berkeleyskypilot/skypilot-debug /bin/bash
conda install gh --channel conda-forge
gh pr checkout 2514
pip install -e .
sky launch --cloud aws --cpus 2 --down echo hi
sky status -r

As this is an update in our dependency, I will also request one more approval from our teammates @concretevitamin @romilbhardwaj @infwinston : )

@Michaelvll Michaelvll mentioned this pull request Sep 5, 2023
8 tasks
Michaelvll added a commit that referenced this pull request Sep 5, 2023
@romilbhardwaj
Copy link
Collaborator

romilbhardwaj commented Sep 5, 2023

I think we want to explicitly disallow [5.3.1 < pyyaml <= 5.4.1] since that may cause issues with cython3. Wondering if we should pin to pyyaml >= 6?

Thanks! I'll run some quick tests on this.

@steve-marmalade
Copy link
Contributor Author

steve-marmalade commented Sep 5, 2023

@romilbhardwaj , my only concern about requiring pyyaml >= 6 is whether this would introduce pyyaml version conflicts between other third-party packages that are pinned to lower version of pyyaml. That said, since pyyaml 6 came out in 2021, I'd hope there are relatively few such issues at this point!

Let me know if you'd like me to push that change, and of course feel free to add additional commits yourself 🙏

Copy link
Collaborator

@romilbhardwaj romilbhardwaj left a comment

Choose a reason for hiding this comment

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

Thanks @steve-marmalade! Left a comment on disallowing pyyaml versions that break with cython3.

sky/setup_files/setup.py Outdated Show resolved Hide resolved
sky/setup_files/setup.py Show resolved Hide resolved
@steve-marmalade
Copy link
Contributor Author

@romilbhardwaj thanks for the review, should be ready for another look

@romilbhardwaj
Copy link
Collaborator

Great, thanks for the quick turnaround @steve-marmalade. I'm running some smoke tests, should be good to go if they pass.

@romilbhardwaj
Copy link
Collaborator

Smoke tests pass, this should be good to go. Thanks for taking the initiative on this @steve-marmalade!

Tested:

  • pytest tests/test_smoke.py

@romilbhardwaj romilbhardwaj merged commit 7d59ff9 into skypilot-org:master Sep 6, 2023
@steve-marmalade
Copy link
Contributor Author

@romilbhardwaj , how would you feel about releasing a 0.3.4 with this change, so that it can be propagated downstream (e.g. to runhouse)? No worries if it's a lot of work, I can use the nightly.

@romilbhardwaj
Copy link
Collaborator

@steve-marmalade we're planning to cut a new release ~2 weeks from now. Would that work? Thanks for your patience with this :)

Michaelvll added a commit that referenced this pull request Sep 8, 2023
* cleaup some dependencies

* Remove upper bound for click

* upgrade actions

* upgrade requirements docs

* Add test for poetry --lock

* fix poetry test

* rename poetry test

* fix

* fix path

* fix github env var

* fix

* fix poetry

* Adopt changes from #2514

* increase poetry build time
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