Add support to dill serializer #406

GregoirePelegrin · 2024-06-26T12:34:44Z

Fixes #405.
Must be merged along with celery/celery#9100

GregoirePelegrin · 2024-06-26T13:34:18Z

I can't seem to find the requirements file to add dill>=0.3.8 in it...

auvipy

I'm thinking of compatibility between dill and pickle

auvipy

E ModuleNotFoundError: No module named 'dill'

GregoirePelegrin · 2024-06-30T19:17:06Z

Yes, as I've stated in my previous comment, I've failed to locate the requirements file. I haven't been able to include dill, which is the cause of this error. When it comes to the compatibility, the admittedly limited tests I've done have thus far been successful. This said, I am not aware of every uses made by the community, and I only tested simple objects.

Nusnus · 2024-06-30T22:45:19Z

Converting to draft until the PR is ready and working.

Thank you 🙏

GregoirePelegrin · 2024-07-01T06:32:22Z

Converting to draft until the PR is ready and working.

Thank you 🙏

I am currently searching for the place to add the dill requirement. It seems there are no packages imported for now, so no requirements.txt exists for the package (not for the tests). I assume that adding the dependency to the test environment would solve the testing issue but would lead to errors when trying to use the package itself. I am not sure how the build works here, so if someone was able to help me with this, I'd be grateful!

GregoirePelegrin · 2024-07-01T09:23:17Z

According to the test suite, it seems it is ready for review. Please tell me if it isn't!

auvipy · 2024-07-02T07:23:54Z

setup.py

@@ -166,6 +166,42 @@ def _is_build_command(argv=sys.argv, cmds=('install', 'build', 'bdist')):
            return arg


+def _strip_comments(l):


are these changes a must have?

These changes are at least partially needed to include the dill library. I used the setup.py of the celery project to make this. There may not be a need to this, I thought it best to make something coherent with such a closely related project. I am open to discussion though.

I think this change brings to light a design flaw with the PR.
I understand using pickle is limiting, but it appears to be a reason why there weren't any requirement.txt files, ever (except for the tests).
Adding a dependency (for dill in this case) might be “against the design” of billiard, which is why there wasn’t any support for new dependencies in the setup.py in the first place.

it appears to be a reason why there weren't any requirement.txt files, ever (except for the tests)

What would be the reason? I must say that it isn't obvious from the outside. Would it be that it would add a lot of lines to the setup.py? I think not for it doesn't seem a valid reason, unless I am missing something.

I think this change brings to light a design flaw with the PR

I must say I don't understand why adding a dependency would be a design flaw here, especially as it is designed to be a drop-in replacement of the currently used one (as you mentioned in the related PR), only adding in capabilities. Again, this would open up the possibility to pass many unsupported types of objects as parameters (while I have only encountered the billiard library when using celery, I expect there are other places it is used, and I cannot imagine a situation where it would be detrimental to add these capabilities).

Adding a dependency (for dill in this case) might be “against the design” of billiard, which is why there wasn’t any support for new dependencies in the setup.py in the first place.

If there is a philosophical issue that I am not aware of in addition to the dependency introduction, I can totally accept it, but regarding this one, it does not seem (to me) that big of a deal. I am well aware that this is ultimately not my decision to make, but is there any underlying reasons this wouldn't be possible?

Is this resolved?

Can I resolve this conversation?

auvipy · 2024-07-02T07:24:48Z

billiard/pool.py

@@ -441,7 +441,7 @@ def _make_recv_method(self, conn):
            if hasattr(conn, 'get_payload') and conn.get_payload:
                get_payload = conn.get_payload

-                def _recv(timeout, loads=pickle_loads):
+                def _recv(timeout, loads=dill_loads):


can you please update or add some tests to check the dill usage?

Could you specify what sort of test you would like added? Pool instanciation is already tested, as well as messages sending and receiving (which should handle the tests on whether this would introduce side effects or not). I have added some tests following this review, but I don't know if they meet your expectations.

Is this resolved?

Can I resolve this conversation?

Nusnus · 2024-07-03T10:34:50Z

requirements/default.txt

@auvipy notice this is a new file

Is it an issue? Or is this related to the conversation below @auvipy's change request? (If it is related, shouldn't we resolve this one, to avoid having multiple threads open on the same issues?)

Can I resolve this conversation?

Nusnus

It appears this PR only changes the default from pickle to dill, but the matching celery PR celery/celery#9100 is where the dill is actually “requested”.

Shouldn’t the celery use-case be enough so the billiard changes won’t be needed?
This is a follow-up to my other comment: https://github.com/celery/billiard/pull/406/files#r1663972900

GregoirePelegrin · 2024-07-03T15:10:25Z

It appears this PR only changes the default from pickle to dill, but the matching celery PR celery/celery#9100 is where the dill is actually “requested”.

Shouldn’t the celery use-case be enough so the billiard changes won’t be needed? This is a follow-up to my other comment: https://github.com/celery/billiard/pull/406/files#r1663972900

It is necessary to have the dill deserializer here, as it would otherwise be unable to process the message sent by the dill serializer in the matching celery PR celery/celery#9100. I initially only modified (locally) the celery library but the error I was trying to remove was then raised here:

billiard/billiard/pool.py

Line 444 in 81cc942

def _recv(timeout, loads=pickle_loads):

This is what led me to also modify the billiard deserializer.

Fix celery#405

c7ebbc0

GregoirePelegrin changed the title ~~Fix #405~~ Fix https://github.com/celery/billiard/issues/405 Jun 26, 2024

GregoirePelegrin changed the title ~~Fix https://github.com/celery/billiard/issues/405~~ Add support to dill serializer Jun 26, 2024

GregoirePelegrin mentioned this pull request Jun 26, 2024

Improves capacity to pass complex arguments to tasks celery/celery#9100

Open

auvipy reviewed Jun 29, 2024

View reviewed changes

auvipy requested changes Jun 29, 2024

View reviewed changes

Nusnus marked this pull request as draft June 30, 2024 22:43

Adding dill in requirements

2dc2441

GregoirePelegrin marked this pull request as ready for review July 1, 2024 09:22

GregoirePelegrin requested a review from auvipy July 1, 2024 09:23

auvipy requested changes Jul 2, 2024

View reviewed changes

Nusnus reviewed Jul 3, 2024

View reviewed changes

Nusnus requested changes Jul 3, 2024

View reviewed changes

Adding dill-specific tests

ab98e7c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to dill serializer #406

Add support to dill serializer #406

GregoirePelegrin commented Jun 26, 2024 •

edited

Loading

GregoirePelegrin commented Jun 26, 2024

auvipy left a comment

auvipy left a comment

GregoirePelegrin commented Jun 30, 2024

Nusnus commented Jun 30, 2024

GregoirePelegrin commented Jul 1, 2024

GregoirePelegrin commented Jul 1, 2024

auvipy Jul 2, 2024

GregoirePelegrin Jul 3, 2024

Nusnus Jul 3, 2024

GregoirePelegrin Jul 4, 2024 •

edited

Loading

GregoirePelegrin Jul 11, 2024

GregoirePelegrin Jul 18, 2024

auvipy Jul 2, 2024

GregoirePelegrin Jul 4, 2024

GregoirePelegrin Jul 11, 2024

GregoirePelegrin Jul 18, 2024

Nusnus Jul 3, 2024

GregoirePelegrin Jul 4, 2024 •

edited

Loading

GregoirePelegrin Jul 18, 2024

Nusnus left a comment

GregoirePelegrin commented Jul 3, 2024

		@@ -166,6 +166,42 @@ def _is_build_command(argv=sys.argv, cmds=('install', 'build', 'bdist')):
		return arg


		def _strip_comments(l):

Add support to dill serializer #406

Are you sure you want to change the base?

Add support to dill serializer #406

Conversation

GregoirePelegrin commented Jun 26, 2024 • edited Loading

GregoirePelegrin commented Jun 26, 2024

auvipy left a comment

Choose a reason for hiding this comment

auvipy left a comment

Choose a reason for hiding this comment

GregoirePelegrin commented Jun 30, 2024

Nusnus commented Jun 30, 2024

GregoirePelegrin commented Jul 1, 2024

GregoirePelegrin commented Jul 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GregoirePelegrin Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GregoirePelegrin Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nusnus left a comment

Choose a reason for hiding this comment

GregoirePelegrin commented Jul 3, 2024

GregoirePelegrin commented Jun 26, 2024 •

edited

Loading

GregoirePelegrin Jul 4, 2024 •

edited

Loading

GregoirePelegrin Jul 4, 2024 •

edited

Loading