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

Fix Task.__repr__() of unpickled object #11463

Merged
merged 2 commits into from
Oct 29, 2024

Conversation

pentschev
Copy link
Member

#11378 added a Task._repr object that is defined during Task.__init__(). In Task.__repr__() the object is tested but it doesn't account for the situation where Task may have been deserialized and thus Task.__init__() is never called, causing that to fail as the attribute was never defined. One other way to fix this would have been to define a default value when the attribute is defined in the scope of the class, but that unfortunately doesn't work due to the __slots__ = tuple(__annotations__) in the class. Another alternative to the hasattr check could be to define a @property with a different name and make __repr__() simply return that, which would handle checks regardless of __init__() being called, but I think that's too much bloat for little benefit.

  • Tests added / passed
  • Passes pre-commit run --all-files

Sorry, something went wrong.

@pentschev
Copy link
Member Author

@fjetter @phofl would you mind having a look since you're author and approver of the PR that introduced this behavior?

Copy link
Contributor

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

     13 files  ± 0       13 suites  ±0   3h 57m 38s ⏱️ + 16m 6s
 13 241 tests + 1   12 179 ✅ + 1   1 062 💤 ±0  0 ❌ ±0 
138 140 runs  +13  119 051 ✅ +14  19 089 💤  - 1  0 ❌ ±0 

Results for commit 85b1c0d. ± Comparison against base commit a0783a8.

Copy link
Member

@fjetter fjetter left a comment

Choose a reason for hiding this comment

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

Thanks! Already saw that something was broken but didn't have the time to look into it

@fjetter fjetter merged commit 206359f into dask:main Oct 29, 2024
25 checks passed
@pentschev
Copy link
Member Author

Thanks @fjetter !

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