-
Notifications
You must be signed in to change notification settings - Fork 54
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
Exclude fsspec.open_kwargs["client_kwargs"]
from pattern hash computation
#444
base: master
Are you sure you want to change the base?
Exclude fsspec.open_kwargs["client_kwargs"]
from pattern hash computation
#444
Conversation
I'm not sure yet what's causing the test failures. I added a test function to |
Thanks @derekocallaghan. On first read, both the fix and the test look quite thorough and appropriate to me. I'm also unclear on what's causing the test failure. It seems possibly unrelated to this PR. |
# hard-to-hash objects, e.g. aiohttp.ClientTimeout. | ||
# This avoids subsequent errors in | ||
# `serialization.either_encode_or_hash()`. | ||
if k != "client_kwargs" |
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.
Could there be the ssl
keyword included in this filtering? It causes similar troubles during hashing and my guess is that it's a similar issue: #418 ?
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.
It may be easiest to exclude fsspec_open_kwargs
itself from the hash computation, rather than filtering a key list containing client_kwargs
etc. @cisaacstern, what do you think?
I had a quick look at the stack trace in #418 (comment) and it looks like a separate problem related to pickle. The expected error associated with this PR issue should be raised in pangeo-forge-recipes.serialization.either_encode_or_hash()
:
raise TypeError(f"object of type {type(obj).__name__} not serializable") |
e.g.
Input In [5], in either_encode_or_hash(obj)
13 elif isinstance(obj, bytes):
14 return obj.hex()
---> 15 raise TypeError(f"object of type {type(obj).__name__} not serializable")
TypeError: object of type ClientTimeout not serializable
As mentioned by @cisaacstern, the failing tests appear to be unrelated to these PR changes. More details in #451 (comment) |
Fixes #443 using the proposed solution. Corresponding test cases added.