Skip to content

ENH: Using named arguments in an iteration of a parametrization. #206

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

Closed
tobiasraabe opened this issue Jan 26, 2022 · 12 comments · Fixed by #229
Closed

ENH: Using named arguments in an iteration of a parametrization. #206

tobiasraabe opened this issue Jan 26, 2022 · 12 comments · Fixed by #229
Labels
enhancement New feature or request

Comments

@tobiasraabe
Copy link
Member

Is your feature request related to a problem?

Currently, each iteration of a parametrization is usually a tuple, but we would like to have name arguments instead. It is easier to read especially when there are many arguments and the order of the arguments can be ignored.

Describe the solution you'd like

  • Related to pytest, we could introduce pytask.param which receives either args or kwargs and represents one iteration. It also has an id parameter to change the id of the iteration.

  • We could also allow for dictionary inputs to each iteration, checking that the keys match the function arguments.

API breaking implications

None, the old style will be preserved.

Describe alternatives you've considered

None

@tobiasraabe tobiasraabe added the enhancement New feature or request label Jan 26, 2022
@tobiasraabe tobiasraabe changed the title ENH: Using named arguments in an iteration or a parametrization. ENH: Using named arguments in an iteration of a parametrization. Jan 26, 2022
@tobiasraabe tobiasraabe added this to the v0.1.9 milestone Feb 17, 2022
@tobiasraabe
Copy link
Member Author

tobiasraabe commented Feb 18, 2022

Discussion at pytest

There is a lot of discussion about this over at pytest.

  • https://github.com/pytest-dev/pytest/issues/5487
  • https://github.com/pytest-dev/pytest/pull/5850
  • https://github.com/pytest-dev/pytest/issues/7568
  • https://github.com/pytest-dev/pytest/issues/9216

The main thing to keep in mind is that they already have two mechanisms to describe parametrizations.

  1. The usual one where each iteration is a tuple of values for each parametrized argument or a single value per iteration if the test has only one parametrized argument.
  2. They have pytest.param(*values, id, marks).

The main arguments against dictionaries in the first approach:

  • The signature is already (type) complicated enough.
  • The same can be achieved with NamedTuple.
  • Dictionaries may lead the user to believe that id is a reserved key to change the name of the iteration of the task.

The main arguments against **kwargs for pytest.param:

  • id and marks are already taken and what to do when they are used in the parametrization?
  • future keywords are blocked if backwards-compatible.

@tobiasraabe
Copy link
Member Author

Workarounds

Named arguments in parametrization

typing.NamedTuple or collections.namedtuple can be used to achieve named arguments per iteration of the parametrization.

from pathlib import Path
from typing import NamedTuple


class Task(NamedTuple):
    depends_on: Path
    produces: Path


@pytask.mark.parametrize("depends_on, produces", [
    Task(depends_on="first_dataset.pkl", produces="first_plot.png"),
    Task(depends_on="second_dataset.pkl", produces="second_plot.png"),
])
def task_plot_data(depends_on, produces):
    df = pd.read_pickle(depends_on)
    ax = df.plot()
    plt.savefig(produces)

Ids closer to iterations

This is a workaround for connecting ids of tasks closer to its values with dictionaries. In combination with NamedTuple the values can be named as well.

tasks = {
    "task_1": ("first_dataset.pkl", "first_plot.png"),
    "task_2": ("second_dataset.pkl", "second_plot.png"),
}


@pytask.mark.parametrize("depends_on, produces", tasks.values(), ids=tasks.keys())
def task_plot_data(depends_on, produces):
    df = pd.read_pickle(depends_on)
    ax = df.plot()
    plt.savefig(produces)

@tobiasraabe
Copy link
Member Author

@hmgaudecker @janosg @roecla @timmens What do you think about the issue and the two workarounds?

I like about the namedtuple approach that I dont have to offer a new API, but I confess that for each complicated task you have to write the namedtuple instead of using dictionaries directly.

I am in favor of implementing pytask.param not only because it would connect ids closer to iterations, but also because it allows markers to be passed to specific iterations. Although this feature was not requested yet and I dont use it often (read at all) with pytest.

We could also buy us some time by just documenting the namedtuple workaround and wait for/collect more feedback.

@hmgaudecker
Copy link
Contributor

hmgaudecker commented Feb 21, 2022

Thanks for your research and ideas!

I am in favor of implementing pytask.param not only because it would connect ids closer to iterations, but also because it allows markers to be passed to specific iterations.

Yes, sounds great.

Although this feature was not requested yet and I dont use it often (read at all) with pytest.

I simply was not aware of that and I think the use cases of pytest / pytask are very different here.

  • In pytest, parametrization is relatively rare, at least in our area of application. With data, you would just pass in an array. If you expect different behaviour, you write different tests.
  • In pytask, parametrization is the norm in reasonably complex projects. Plus, dependencies and targets become very complicated quickly. E.g., in the project for which I use pytask mostly at the moment, the usual thing would be 8 deps and 2 targets run for 2-4 different model specifications. Hard to find that in typical test suites.

So pytask.param would be awesome! I would actually go one step further (mainly for selling and to save me from doing that myself in every other project....) and provide something like pytask.convert_dict_to_parametrization. Deliberately too long name, but I think two statements are true

  1. The above reservations against keyworks and dicts make perfect sense for a library like pytest, which is used mostly by professional developers.
  2. The above reservations are not serious enough for pytask, where it would lower adoption a lot IMO.

Behaviour would be:

  • Allow to write a dict like:
my_tasks = {
    "task_1": {
        "depends_on": {
            "first_dataset.pkl",
            "some_specification.yaml",
        },
        "produces": "first_plot.png"
    },
    "task_2": {
        "depends_on": "second_dataset.pkl",
        "produces": "second_plot.png", 
        "marks": pytask.mark.skip
    },
}
  • Convert to list of pytask.params (fill up id with outermost keys if not explicitly provided in dict). Pass everything that is not id or marks to parametrize
  • Collect keys of second level in dictionaries, build first parametrize arg as ",".join(*d.keys())
  • Promise to add future keywords only in new major version, provide check before upgrading (should be trivial and I am willing to wager a non-negligible bet that it will end up as the empty set for the next three-five- years).
@pytask.mark.parametrize(*pytask.convert_dict_to_parametrization(my_tasks))
def task_plot_data(depends_on, produces):
    df = pd.read_pickle(depends_on)
    ax = df.plot()
    plt.savefig(produces)

How does that sound?

@hmgaudecker
Copy link
Contributor

(and clearly advertise that as a convenience function, not a core pytask component)

@janosg
Copy link

janosg commented Feb 23, 2022

I like the focus on IDs in the dictionary approach and Hans-Martin's proposed Solution.

One thing I don't like about NamedTuples as args is that despite having names, the order of the args is not automatically aligned with the function signature. However, this is something that you could check during the collection phase and raise an error.

I had another look at ward and really like their way of parametrising tests. Pytest/Pytask style parametrizations are very confusing for beginners (for me personally and most students in courses). The ward style seems much simpler because it hides the lazy evaluation of test functions. I also think that a lot of the complexity discussed here comes exactly from the separation of function calls and definition of inputs with which they are called.

Of course, this would be a paradigm shift for pytask as it would move away from pytest and experienced pytest users would have to learn something new to use pytask.

It is also potentially hard to implement. I guess it requires that in the collection phase all task functions are mocked with some tracer object that records with which argument they were called. Without the decorators that are required in ward this is probably even harder to achieve.

@hmgaudecker
Copy link
Contributor

Maybe I am missing something, but I am not sure how ward-style parametrizations would look easier in the pytask case, where we are usually not talking about 1, 2, 3 but each element being a rather long list of deps and targets? What I like about the above dict is that everything is named and it keeps deps and targets closer together, without having to think about the order.

But I am probably missing something fundamental. Maybe you could translate the above dict to how it would look like ward-style? Old man needs examples. 🤷

@tobiasraabe
Copy link
Member Author

tobiasraabe commented Feb 23, 2022

Love the idea about the check for whether namedtuples are consistent with the signature!

What do you mean by ward hides the lazy evaluation of test functions? Do you mean the for-loop around the task function?

I am not sure I like wards way apart from the for-loop.

  1. I assumed intuitively that multiple each form a cartesian product.
  2. each makes it harder to identify a single test case especially with many parametrizations.
  3. not really valid, but I would also guess that it is kind of fiddly to parse test cases from the function signature 😄.

I think wards way of writing tests is somewhat inspired by how you would do it in java script. Meaning the focus on ids as decorators and anonymous test functions.

What's nice about this is that you will never have nameclashes. You can assign different internal ids to a test with the same id in the same module.

@janosg
Copy link

janosg commented Feb 23, 2022

I just mean the for loop instead of a parametrize decorator. I would leave everything else as is. That's also why I mentioned that implementation could be tricky if there is no decorator to mark tasks.

By hiding lazy evaluation I mean exactly the for loop. In pytask you specify functions and arguments with which they will be called at some point. In ward you just call functions in a loop. Whether you construct arguments inside the loop or beforehand becomes your choice.

Edit: In ward you don't call the functions in a loop, but you define them setting default values for all arguments, so they can be called very easily later.

@tobiasraabe
Copy link
Member Author

tobiasraabe commented Feb 23, 2022

We have a task decorator which could be extended. And offer two interfaces.

Actually, I really like the last point about it because instead of preventing users from making mistakes and informing them about them, dont let them do mistakes.

@tobiasraabe
Copy link
Member Author

tobiasraabe commented Feb 23, 2022

@hmgaudecker, Janos and I had a very short talk about ward's approach to parametrizations by looping over the task function and we think this is the easiest interface one could imagine.

Your dictionary approach just needs the extra lines for the looping over the function and unpacking the dictionary with **kwargs.

Additionally, I think this approach renders

obsolete which is a clear win.

Additionally, I would open a ticket for documenting namedtuples and adding a check that attribute names match the signature of the parametrization.

What are your thoughts?

@hmgaudecker
Copy link
Contributor

I don't see through all that, but I trust you guys.

All I want is to use keywords everywhere (no reliance on order) without having to define a NamedTuple myself. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants