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

Add support to dill serializer #406

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions billiard/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import signal
import sys

import dill
import pickle

from .exceptions import RestartFreqExceeded
Expand All @@ -14,6 +15,9 @@
pickle_load = pickle.load
pickle_loads = pickle.loads

dill_load = dill.load
dill_loads = dill.loads

# cPickle.loads does not support buffer() objects,
# but we can just create a StringIO and use load.
from io import BytesIO
Expand Down
6 changes: 3 additions & 3 deletions billiard/pool.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
from . import cpu_count, get_context
from . import util
from .common import (
TERM_SIGNAL, human_status, pickle_loads, reset_signals, restart_state,
TERM_SIGNAL, human_status, dill_loads, reset_signals, restart_state,
)
from .compat import get_errno, mem_rss, send_offset
from .einfo import ExceptionInfo
Expand Down Expand Up @@ -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):
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Is this resolved?

Copy link
Author

Choose a reason for hiding this comment

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

Can I resolve this conversation?

return True, loads(get_payload())
else:
def _recv(timeout): # noqa
Expand All @@ -456,7 +456,7 @@ def _recv(timeout): # noqa
return False, None
return _recv

def _make_child_methods(self, loads=pickle_loads):
def _make_child_methods(self, loads=dill_loads):
self.wait_for_job = self._make_protected_receive(self.inq)
self.wait_for_syn = (self._make_protected_receive(self.synq)
if self.synq else None)
Expand Down
1 change: 1 addition & 0 deletions requirements/default.txt
Copy link
Member

Choose a reason for hiding this comment

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

@auvipy notice this is a new file

Copy link
Author

@GregoirePelegrin GregoirePelegrin Jul 4, 2024

Choose a reason for hiding this comment

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

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?)

Copy link
Author

Choose a reason for hiding this comment

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

Can I resolve this conversation?

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
dill>=0.3.8
37 changes: 37 additions & 0 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,42 @@ def _is_build_command(argv=sys.argv, cmds=('install', 'build', 'bdist')):
return arg


def _strip_comments(l):
Copy link
Member

Choose a reason for hiding this comment

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

are these changes a must have?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

@GregoirePelegrin GregoirePelegrin Jul 4, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

Is this resolved?

Copy link
Author

Choose a reason for hiding this comment

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

Can I resolve this conversation?

return l.split('#', 1)[0].strip()


def _pip_requirement(req):
if req.startswith('-r '):
_, path = req.split()
return reqs(*path.split('/'))
return [req]


def _reqs(*f):
return [
_pip_requirement(r) for r in (
_strip_comments(l) for l in open(
os.path.join(os.getcwd(), 'requirements', *f)).readlines()
) if r]


def reqs(*f):
"""Parse requirement file.

Example:
reqs('default.txt') # requirements/default.txt
reqs('extras', 'redis.txt') # requirements/extras/redis.txt
Returns:
List[str]: list of requirements specified in the file.
"""
return [req for subreq in _reqs(*f) for req in subreq]


def install_requires():
"""Get list of requirements required for installation."""
return reqs('default.txt')


def run_setup(with_extensions=True):
extensions = []
if with_extensions:
Expand Down Expand Up @@ -204,6 +240,7 @@ def run_setup(with_extensions=True):
maintainer=meta['maintainer'],
maintainer_email=meta['contact'],
url=meta['homepage'],
install_requires=install_requires(),
zip_safe=False,
license='BSD',
python_requires='>=3.7',
Expand Down
Loading