Use attrs instead of dataclasses #174
Conversation
|
@djarecka @effigies - this is very much a WIP to go in parallel with #173. i have only run i do think even based on this, that attrs will simplify a lot of the code. in this particular instance i'm doing a very superficial change (keeping the pickling stuff for instance). but overall it seems quite straightforward to move over. here is what i have used. allows better inheritance. everything is a keyword arg, and defaults work (@djarecka - so default_value may need to be removed or altered) |
Codecov Report
@@ Coverage Diff @@
## master #174 +/- ##
==========================================
- Coverage 86.72% 86.71% -0.02%
==========================================
Files 17 17
Lines 2644 2702 +58
Branches 680 700 +20
==========================================
+ Hits 2293 2343 +50
- Misses 228 230 +2
- Partials 123 129 +6
Continue to review full report at Codecov.
|
|
@djarecka - just a note that |
|
@satra - great! I tried to read this quickly, but I'd need more time to understand all changes, will do it later |
|
@djarecka - for me all tests pass under engine except 3 in |
|
@satra - i can take a look |
…in the input hash property - only tuple are converted to lists
small fix to the failing tests
|
@effigies and @djarecka - i am opening this PR for discussion before we merge. i am going to vote yes since attrs appears to have a nice approach to validation as well, which can be globally disabled. so this should allow us to do type validation quite easily. as far as i could tell, attrs still does not address the pickling issue. |
| if modified_inputs is not None: | ||
| self.inputs = attr.evolve(self.inputs, **modified_inputs) | ||
| cmdline = " ".join(self.container_args + self.command_args) | ||
| self.inputs = attr.evolve(self.inputs, **orig_inputs) |
There was a problem hiding this comment.
was this needed to pass the existing tests?
There was a problem hiding this comment.
this was updated to reflect the fact that one can call cmdline before calling run. and this property is not used for running.
but to answer your original question, i don't think there was a test that tested this.
There was a problem hiding this comment.
I'm trying to find situation when your modification changes anything. If you think about a single task and running cmdline before run, this worked without your changes. If you think about workflows, this doesn't work, there are still LF. Could you give me some examples or revert the changes for now.
There was a problem hiding this comment.
if you prefer to keep it, I'll just make a note to think about it later, but couldn't find a good example
There was a problem hiding this comment.
you will also have to take the changes away from run. to test the issue, take the current master branch, print cmdline from a container task, run it, and then print cmdline again.
There was a problem hiding this comment.
in master this modified test will fail. (i'll push it to this branch)
def test_docker_inputspec_3(plugin, tmpdir):
""" input file is in the container, so metadata["container_path"]: True,
the input will be treated as a str """
filename = "/proc/1/cgroup"
cmd = "cat"
my_input_spec = SpecInfo(
name="Input",
fields=[
(
"file",
File,
dc.field(
metadata={
"mandatory": True,
"position": 1,
"help_string": "input file",
"container_path": True,
}
),
)
],
bases=(DockerSpec,),
)
docky = DockerTask(
name="docky",
image="busybox",
executable=cmd,
file=filename,
input_spec=my_input_spec,
strip=True,
)
cmdline = docky.cmdline # in master this will fail
res = docky()
assert "docker" in res.output.stdout
assert cmdline == docky.cmdlineThere was a problem hiding this comment.
I really don't see a difference between the old cmdline and the new one: you can see: satra#12
|
sure, let's change it! |
|
I'm okay with changing it. Also, do we have a precise description of what the pickling problem is? |
concurrent futures uses regular pickle, so cloudpickle cannot be used. |
i'll let @djarecka hit the merge button once she is ok with it. i'll make a separate dataclasses branch from master in case we wan't to easily find it. |
| @@ -10,7 +10,7 @@ A simple dataflow engine with scalable semantics. | |||
| The goal of pydra is to provide a lightweight Python dataflow engine for DAG construction, manipulation, and distributed execution. | |||
|
|
|||
| Feature list: | |||
| 1. Python 3.7+ using type annotation and dataclasses | |||
| 1. Python 3.7+ using type annotation and [attrs](https://www.attrs.org/en/stable/) | |||
There was a problem hiding this comment.
Do we still use 3.7+ features, or can we relax to 3.6? (I know that at this point 3.7 isn't cutting edge, so maybe it doesn't matter.)
There was a problem hiding this comment.
my current intent was to get this merged, then do additional trimming/testing/relaxing. i did mention the 3.6 expansion to dorota when we met.
|
It run this is needed... i only argu that nothing changes in this place
…On Fri, Jan 17, 2020, 21:12 Satrajit Ghosh ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pydra/engine/task.py
<#174 (comment)>:
> @@ -385,7 +400,13 @@ def __init__(
@Property
def cmdline(self):
"""Get the actual command line that will be submitted."""
- return " ".join(self.container_args + self.command_args)
+ orig_inputs = attr.asdict(self.inputs)
+ modified_inputs = template_update(self.inputs)
+ if modified_inputs is not None:
+ self.inputs = attr.evolve(self.inputs, **modified_inputs)
+ cmdline = " ".join(self.container_args + self.command_args)
+ self.inputs = attr.evolve(self.inputs, **orig_inputs)
you will also have to take the changes away from run. to test the issue,
take the current master branch, print cmdline from a container task, run
it, and then print cmdline again.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#174?email_source=notifications&email_token=AAMV6GR6EE2HKSPJQ2JL7P3Q6JQPRA5CNFSM4KFUQGI2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCSHQVQY#discussion_r368199140>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAMV6GUMTGWCKJS3MONACGTQ6JQPRANCNFSM4KFUQGIQ>
.
|
Types of changes
Summary
Switch to attrs from dataclasses
Checklist
(we are using
black: you canpip install pre-commit,run
pre-commit installin thepydradirectoryand
blackwill be run automatically with each commit)Acknowledgment