Implement a new loop-based approach to parametrizations. #229

tobiasraabe · 2022-02-28T23:59:51Z

Closes #206, resolves #132.

Changes

This PR implements a new loop-based approach to parametrizations.

Todo

Add better typing for functions with pytask_meta attribute.
Document the new approach to parametrizations.

Maybes interesting for typing: https://github.com/python/mypy/issues/2087#issuecomment-432963659.

codecov · 2022-03-01T00:02:04Z

Codecov Report

Merging #229 (caaaa91) into main (8c0b2cf) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #229      +/-   ##
==========================================
+ Coverage   94.30%   94.37%   +0.07%     
==========================================
  Files          77       79       +2     
  Lines        6843     6956     +113     
==========================================
+ Hits         6453     6565     +112     
- Misses        390      391       +1

Flag	Coverage Δ
end_to_end	`80.18% <97.91%> (+0.31%)`	⬆️
integration	`41.76% <36.11%> (-0.21%)`	⬇️
unit	`69.39% <43.75%> (-0.61%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/_pytask/__init__.py	`66.66% <ø> (ø)`
tests/test_parametrize.py	`98.23% <ø> (-0.07%)`	⬇️
src/_pytask/collect.py	`94.47% <100.00%> (+0.06%)`	⬆️
src/_pytask/hookspecs.py	`100.00% <100.00%> (ø)`
src/_pytask/mark/structures.py	`93.84% <100.00%> (+0.29%)`	⬆️
src/_pytask/models.py	`100.00% <100.00%> (ø)`
src/_pytask/parametrize.py	`100.00% <100.00%> (ø)`
src/_pytask/parametrize_utils.py	`100.00% <100.00%> (ø)`
src/_pytask/task.py	`100.00% <100.00%> (ø)`
src/_pytask/task_utils.py	`100.00% <100.00%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c0b2cf...caaaa91. Read the comment docs.

tobiasraabe · 2022-03-01T12:33:53Z

Hi @janosg, @hmgaudecker,

I ping you for some intermediate feedback on the implementation before I start polishing it and writing some documentation. You can review the new interface in the test module test_task.py and also add comments elsewhere.

hmgaudecker · 2022-03-01T13:15:26Z

Thanks, just on my way to trying things out... Needed to add tabulate to get anything going, but tests are still failing on my machine.

Any ideas?

tobiasraabe · 2022-03-01T13:19:08Z

Yes, platforms have different limits on how many file handlers can be opened. Every tmp_path fixture is a new file handle Here is more info.

Increase the limit with something like ulimit -n 4048?

hmgaudecker · 2022-03-01T13:25:50Z

Spot on, as always! Thanks!

hmgaudecker · 2022-03-01T13:52:49Z

a.m.a.z.i.n.g.

Nothing else to add.

Other than, perhaps, you'll need to be careful in the documentation not to publish all the different ways how to use that. I'd be all for the kwargs version, of course 😉. I am not sure I want to explain the from_signature version to many people. (though I agree it is beautiful)

Re kwargs: Adding tests for irregular dicts would be good (different keys in round 1 compared to round 2, subset / superset of task function args). And are name and id valid keys?

hmgaudecker · 2022-03-01T14:05:31Z

Other than, perhaps, you'll need to be careful in the documentation not to publish all the different ways how to use that.

What I mean is:

There should be one-- and preferably only one --obvious way to do it.

In general, I am a little scared of a (large) proliferation of ways how you can specify tasks. At least tutorials should focus on one sensible way that scales well IMHO, if power users want to do something else for whatever reason, fine.

janosg · 2022-03-01T15:18:34Z

That was quick! Very nice test cases and very nice new interface!

…the how to guides.

…ytask-dev/pytask into implement-new-parametrizations

hmgaudecker

Looks great, thanks!

One semantic thing: Assuming you want to continue supporting "old" parametrizations, I would call it "the pytest way". Just briefly mention that for experienced pytest users, that may be the most natural approach. But don't add it as the very first thing in the tutorial, or only for a transition period while trying to make the current user base switch.

docs/source/tutorials/how_to_parametrize_a_task.rst

tobiasraabe · 2022-03-03T15:41:20Z

Looks great, thanks!

One semantic thing: Assuming you want to continue supporting "old" parametrizations, I would call it "the pytest way". Just briefly mention that for experienced pytest users, that may be the most natural approach. But don't add it as the very first thing in the tutorial, or only for a transition period while trying to make the current user base switch.

Thanks for your comments. Yes, it was meant for people who are looking for the previous docs and are confused. So, I think it makes sense to have it close to the top of the file for some time.

I think about deprecating pytest's way in the next minor or major version change. Whatever comes first. Removing it now would just annoy everyone. I think I will add a deprecation warning in v0.2.0 saying either to fix pytask<=0.2 or refactor the syntax. And I will add a flag to the configuration which disables the warning for v0.2.

hmgaudecker · 2022-03-03T16:24:38Z

Great. Then just say now that this is the pytest way and that until 0.19, it was the only way? Tobias Raabe ***@***.***> schrieb am Do., 3. März 2022, 16:41:

…

Looks great, thanks! One semantic thing: Assuming you want to continue supporting "old" parametrizations, I would call it "the pytest way". Just briefly mention that for experienced pytest users, that may be the most natural approach. But don't add it as the very first thing in the tutorial, or only for a transition period while trying to make the current user base switch. Thanks for your comments. Yes, it was meant for people who are looking for the previous docs and are confused. So, I think it makes sense to have it close to the top of the file for some time. I think about deprecating pytest's way in the next minor or major version change. Whatever comes first. Removing it now would just annoy everyone. I think I will add a deprecation warning in v0.2.0 saying either to fix pytask<=0.2 or refactor the syntax. And I will add a flag to the configuration which disables the warning for v0.2. — Reply to this email directly, view it on GitHub <#229 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABGV6A32XYR5E22ATB6LMLU6DMTJANCNFSM5PSQA35A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

…tion since an arg is missing.

tobiasraabe · 2022-03-03T18:16:19Z

@hmgaudecker and @janosg, this PR is ready for a final review if you like. No need to do it actually.

hmgaudecker · 2022-03-03T19:34:58Z

Thanks, indeed I don't think I could provide anything useful at this stage anymore 😊. Amazing stuff. Tobias Raabe ***@***.***> schrieb am Do., 3. März 2022, 19:16:

…

@hmgaudecker <https://github.com/hmgaudecker> and @janosg <https://github.com/janosg>, this PR is ready for a final review if you like. No need to do it actually. — Reply to this email directly, view it on GitHub <#229 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABGV6HGYQB2HZ7ZOOBB5JLU6D6X5ANCNFSM5PSQA35A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

janosg

This is an amazing change and a very clean implementation!

When you remove "the pytest way" I think it would be good to leave in an error message that tells you the last version of pytask supporting the pytest way. Otherwise people who do not follow pytask closely and need to re-run an old project for a revision might be lost.

With these and some other changes, pytask has stopped being a pytest clone for tasks. You might want to adjust the documentation, readme and short descriptions in that direction.

docs/source/tutorials/how_to_parametrize_a_task.rst

tobiasraabe added 2 commits February 27, 2022 22:06

Implement a couple of tests and add to changes.

e66bf07

Implement functionality to make tests pass.

5390091

tobiasraabe added this to the v0.2.0 milestone Feb 28, 2022

tobiasraabe added 7 commits March 1, 2022 01:08

Fix errors.

aca9f60

Make name of task deco positional, rest kwargs, and add test for id.

9cc8007

Implement id as a kwarg to the task marker.

2819db6

Implement parsing of args where id components are stringified.

ab69519

Add task testing error handling.

29c4170

fix tests.

93e52d0

Extend docstring.

8b296a2

Add tabulate to environment.

3636716

tobiasraabe added 6 commits March 3, 2022 01:02

Rewrite tutorial and move explanation on @pytask.mark.parametrize to …

d1906ec

…the how to guides.

Update the best-practices guide on parametrizations:

7581871

Fix link error.

ba912f4

fix.

b7975be

fix.

ebb5dc9

Merge branch 'implement-new-parametrizations' of https://github.com/p…

69054b9

…ytask-dev/pytask into implement-new-parametrizations

hmgaudecker reviewed Mar 3, 2022

View reviewed changes

docs/source/tutorials/how_to_parametrize_a_task.rst Outdated Show resolved Hide resolved

docs/source/tutorials/how_to_parametrize_a_task.rst Outdated Show resolved Hide resolved

tobiasraabe added 2 commits March 3, 2022 18:38

Integrate comments from HMG.

07994af

Clarify returnining None in the wrapper function.

7084b11

tobiasraabe added 3 commits March 3, 2022 18:51

Remove function type with pytask_meta defined.

1d063b4

Add a test with irregular dicts which causes a TypeError during execu…

e60574c

…tion since an arg is missing.

Highlight parametrizations via loops in README.rst.

58b9365

tobiasraabe mentioned this pull request Mar 3, 2022

Support NamedTuples for named arguments in parametrizations. #224

Closed

2 tasks

janosg approved these changes Mar 4, 2022

View reviewed changes

docs/source/tutorials/how_to_parametrize_a_task.rst Show resolved Hide resolved

Merge branch 'main' into implement-new-parametrizations

a511aad

tobiasraabe mentioned this pull request Mar 7, 2022

ENH: Deprecate @pytask.mark.parametrize. #233

Closed

Rerun ci.

caaaa91

tobiasraabe merged commit 3e41ebd into main Mar 7, 2022

tobiasraabe deleted the implement-new-parametrizations branch March 7, 2022 12:07

Implement a new loop-based approach to parametrizations. #229

Implement a new loop-based approach to parametrizations. #229

Uh oh!

Conversation

tobiasraabe commented Feb 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Todo

Uh oh!

codecov bot commented Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tobiasraabe commented Mar 1, 2022

Uh oh!

hmgaudecker commented Mar 1, 2022 • edited by tobiasraabe Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobiasraabe commented Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hmgaudecker commented Mar 1, 2022

Uh oh!

hmgaudecker commented Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hmgaudecker commented Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janosg commented Mar 1, 2022

Uh oh!

hmgaudecker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tobiasraabe commented Mar 3, 2022

Uh oh!

hmgaudecker commented Mar 3, 2022 via email

Uh oh!

tobiasraabe commented Mar 3, 2022

Uh oh!

hmgaudecker commented Mar 3, 2022 via email

Uh oh!

janosg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tobiasraabe commented Feb 28, 2022 •

edited

Loading

codecov bot commented Mar 1, 2022 •

edited

Loading

hmgaudecker commented Mar 1, 2022 •

edited by tobiasraabe

Loading

tobiasraabe commented Mar 1, 2022 •

edited

Loading

hmgaudecker commented Mar 1, 2022 •

edited

Loading

hmgaudecker commented Mar 1, 2022 •

edited

Loading