-
Notifications
You must be signed in to change notification settings - Fork 252
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
dill>=0.3.8 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -166,6 +166,42 @@ def _is_build_command(argv=sys.argv, cmds=('install', 'build', 'bdist')): | |
return arg | ||
|
||
|
||
def _strip_comments(l): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are these changes a must have? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These changes are at least partially needed to include the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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
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
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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this resolved? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
@@ -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', | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this resolved?
There was a problem hiding this comment.
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?