Skip to content

[MRG] Fix custom reducers registered in dispatch_table handled by a member descriptor #272

Merged
ogrisel merged 8 commits into
joblib:masterfrom
ogrisel:member-descriptor-dispatch-table
Sep 24, 2020
Merged

[MRG] Fix custom reducers registered in dispatch_table handled by a member descriptor #272
ogrisel merged 8 commits into
joblib:masterfrom
ogrisel:member-descriptor-dispatch-table

Conversation

@ogrisel

@ogrisel ogrisel commented Sep 24, 2020

Copy link
Copy Markdown
Contributor

This is a tentative fix for #259 based on something suggested by @pierreglaser in #260 (comment).

However it does not work the way I thought it would. I am opening the PR anyway to share my understanding with @pierreglaser (and other people interested in this problem). The interesting part is that using the member descriptor to set the custom dispatch_table on the pickler is actually changing the class and not the instance.

@ogrisel

ogrisel commented Sep 24, 2020

Copy link
Copy Markdown
Contributor Author

@pierreglaser feel free to close this PR if you agree that this is a dead-end. I will open an alternative PR that delays the init to the super class.

Comment thread loky/backend/reduction.py Outdated

@pierreglaser pierreglaser left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I left a few comments, @ogrisel let me know what you think once you read them.

Comment thread tests/test_cloudpickle_wrapper.py
Comment thread loky/backend/reduction.py
# with an class level dispatch_table attribute.
self._set_dispatch_table(loky_dt)

# Register custom reducers

@pierreglaser pierreglaser Sep 24, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

CustomizablePickler.register still tries to modify self.dispatch_table directly, thus

  • modifying the cloudpickle.Cloudpickler.dispatch_table class attribute
  • not modifying _pickle.Pickler.dispatch_table.__get__(self)
    both because of attribute shadowing.

I suggest we update loky_dt with the reducers dict before calling self._set_dispatch_table.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the explanations, that makes sense. Alternatively we could also setattr on top of set and that should fix the problem. Let me give it a try.

Comment thread loky/backend/reduction.py
# On top of member descriptor set, also use setattr such that code
# that directly access self.dispatch_table gets a consistent view
# of the same table.
self.dispatch_table = dispatch_table

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍

@ogrisel

ogrisel commented Sep 24, 2020

Copy link
Copy Markdown
Contributor Author

Before merging I would like to test with cpython 3.7 + pickle5 which is not tested by the CI.

@ogrisel ogrisel changed the title [NoMRG] member descriptor pickler dispatch_table experiment [NoMRG] Fix custom reducers registered in dispatch_table handled by a member descriptor Sep 24, 2020
@ogrisel

ogrisel commented Sep 24, 2020

Copy link
Copy Markdown
Contributor Author

I get a crash with python 3.7 + pickle5 but this is unrelated to this PR as I also observe this on master:

E               Traceback (most recent call last):
E                 File "/home/ogrisel/code/loky/loky/backend/popen_loky_posix.py", line 197, in <module>
E                   prep_data = pickle.load(from_parent)
E               ValueError: unsupported pickle protocol: 5

@ogrisel ogrisel changed the title [NoMRG] Fix custom reducers registered in dispatch_table handled by a member descriptor [MRG] Fix custom reducers registered in dispatch_table handled by a member descriptor Sep 24, 2020
@ogrisel ogrisel merged commit 5f8c2fd into joblib:master Sep 24, 2020
@ogrisel ogrisel deleted the member-descriptor-dispatch-table branch September 24, 2020 16:09
@pierreglaser

Copy link
Copy Markdown
Collaborator

I get a crash with python 3.7 + pickle5 but this is unrelated to this PR as I also observe this on master

For the record I'm giving fixing this a shot, but it's not trivial. I'll resume working on it if @ogrisel or others deem it as high priority.

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.

2 participants