-
Notifications
You must be signed in to change notification settings - Fork 3
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
Vendor cloudpickle #686
Vendor cloudpickle #686
Conversation
Needed by a pickle compatibility test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me. Other than the minor inline comment, have one question which I'll take offline. Thanks!
@@ -1,6 +1,6 @@ | |||
attrs==23.2.0 | |||
certifi==2024.2.2 | |||
cloudpickle==2.2.1 | |||
cloudpickle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ihnorton I did at one point, and then realized that if tests don't pass with cloudpickle 3 in the environment, we have a major problem. I'm inclined to leave it for now.
Again, I went through every failing test at 6bc48b5. They are all explained by tiledb.cloud._vendor.cloudpickle not existing on the server side. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
* Vendor cloudpickle * Install missing cloudpickle test components * Add cloudpickle back to CI requirements Needed by a pickle compatibility test * In module code, import vendored cloudpickle as tdbcp
All code now does
from tiledb.cloud._vendor import cloudpickle as tdbcp
.