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

[GCP] Avoid dumping cachetools for backward compatibilty #2604

Merged
merged 2 commits into from
Sep 25, 2023

Conversation

Michaelvll
Copy link
Collaborator

@Michaelvll Michaelvll commented Sep 25, 2023

cachetools > 5.0 will have different APIs. Dumping the Cache will cause backward compatibility issues.

To reproduce:

  1. pip install "cachetools<5.0"; sky launch -c test --cloud gcp --cpus 2
  2. pip install "cachetools>=5.0"; sky launch -c test

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (please specify below)
    • Reproducible code above
  • 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

@concretevitamin
Copy link
Collaborator

To reproduce:

  1. pip install "cachetools<5.0"; sky launch -c test --cloud gcp --cpus 2
  2. pip install "cachetools>=5.0"; sky launch -c test

How was this encountered? I feel like protecting against these kinds of out-of-band dependency breakage is hard, and we probably have other issues of this type. That said, ok to let this go in if important.

@Michaelvll
Copy link
Collaborator Author

Michaelvll commented Sep 25, 2023

To reproduce:

  1. pip install "cachetools<5.0"; sky launch -c test --cloud gcp --cpus 2
  2. pip install "cachetools>=5.0"; sky launch -c test

How was this encountered? I feel like protecting against these kinds of out-of-band dependency breakage is hard, and we probably have other issues of this type. That said, ok to let this go in if important.

This can happen when people upgrade their GCP package like google-auth which removes cachetools<5 constraint in their setup in a certain version. This issue is very serious, as after that all the GCP clusters will not be able to be loaded from our database, and a user may fail to find a way to recover it especially if they launched a new cluster with the new cachetools installed, i.e., both old and new version of clusters exist in the db.
I was only able to get myself out of the state by removing the new cluster from my database manually and downgrade my cachetools.

This kind of dependency package issue will only happens when we save the object with the package to the database with pickle.dump, and the package changes its API of object construction, so in the future we should be careful with what to save to the database, and avoid saving third-party objects.

Copy link
Collaborator

@concretevitamin concretevitamin left a comment

Choose a reason for hiding this comment

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

Thanks @Michaelvll, SGTM.

@Michaelvll Michaelvll merged commit ebd19de into master Sep 25, 2023
18 checks passed
@Michaelvll Michaelvll deleted the avoid-saving-cachetools branch September 25, 2023 19:51
jc9123 pushed a commit to jc9123/skypilot that referenced this pull request Oct 11, 2023
…g#2604)

* [GCP] Avoid dumping cachetools for backward compatibilty

* Add comment
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.

None yet

2 participants