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

Exclude fsspec.open_kwargs["client_kwargs"] from pattern hash computation #444

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

derekocallaghan
Copy link
Contributor

Fixes #443 using the proposed solution. Corresponding test cases added.

@derekocallaghan
Copy link
Contributor Author

derekocallaghan commented Nov 30, 2022

I'm not sure yet what's causing the test failures. I added a test function to tests/test_serialization.py, but it wasn't run according to the logs. I wouldn't have thought the code changes would cause the current test failures, I'll look into it.

@cisaacstern
Copy link
Member

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"

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 ?

Copy link
Contributor Author

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

@derekocallaghan
Copy link
Contributor Author

As mentioned by @cisaacstern, the failing tests appear to be unrelated to these PR changes. More details in #451 (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.

Hash computation fails for recipe file patterns containing aiohttp objects
3 participants