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

Exclude combinations from parametrized matrix builds #392

Closed
danieleades opened this issue Mar 1, 2021 · 19 comments · Fixed by #413
Closed

Exclude combinations from parametrized matrix builds #392

danieleades opened this issue Mar 1, 2021 · 19 comments · Fixed by #413

Comments

@danieleades
Copy link

I'm using the @parametrize decorator to create a build matrix, but i've got certain combinations I wish to exclude.

right now i'm using a little escape hatch

if valid_combination(*deps):
    run_tests(session, *deps)
else:
    session.skip("unsupported combination")

would be nice if Nox could provide an API for adding or removing 'cells' from the matrix (similarly to what you can do with github actions)

@theacodes
Copy link
Collaborator

Hmm. How would you like this to work from the command-line perspective?

@danieleades
Copy link
Author

danieleades commented Mar 1, 2021

Hmm. How would you like this to work from the command-line perspective?

I've not really thought about what the API would look like. Can you clarify what you mean by "from the command-line perspective"?

I would think if you try to run a particular session from the command line that is in the 'excluded' list it would behave the same way as if you specified a session that was entirely outside your parametrize range. Having said that, a slightly helpful warning/error would help the unwary figure out why the session doesn't exist.

maybe something like

PYTHON_VERSIONS = ["3.6", "3.7", "3.8"]
MY_DEP_VERSIONS = ["1.0", "2.0"]

@nox.session(python=PYTHON_VERSIONS)
@nox.parametrize("my_dep", MY_DEP_VERSIONS)
@nox.parametrize_exclude({"python": "3.6", "my_dep": "2.0"})
def tests(python, my_dep):
    session.install(f'my_dep=={my_dep}')
    session.run('pytest')

i write this with absolutely 0 knowledge of how these decorators are implemented, so I've no idea if this is sensible

probably the values in @nox.paramatrize_exclude could also be Iterables. as in-

@nox.parametrize_exclude({"python": ["3.5", "3.6"], "my_dep": "2.0"})

which would mean "any session where the python version is in ["3.5", "3.6"] and my_dep is in ["2.0"] is excluded".

Maybe keyword arguments would be more readable than a dict? i dunno.

the use case would be to exclude combinations that you don't care about (for whatever reason), or that you already know don't work and are excluded by your dependency requirements anyway

You could also invert the logic as well, and have an API for adding extra cases to the matrix. parametrise_include, if you will.

bonus points for

  • warning if patterns are excluded that aren't in the matrix anyway
  • warning if pattern excludes the entire matrix
  • warning/error if any parametrize_include statement overlaps with any parametrize_exclude statement (since there's no reason to ever have this)
  • error if pattern includes unrecognised values

@danieleades
Copy link
Author

just to add to that, one workaround would be to have multiple test functions, and manually exclude the cases you weren't interested in.

If i reuse the sketch above it would look something like this-

@nox.session(python="3.6")
@nox.parametrize("my_dep", ["1.0"])
def tests_py36(python, my_dep):
    session.install(f'my_dep=={my_dep}')
    session.run('pytest')

@nox.session(python=["3.7", "3.8"])
@nox.parametrize("my_dep", ["1.0", "2.0"])
def tests(python, my_dep):
    session.install(f'my_dep=={my_dep}')
    session.run('pytest')

but it's a little bit ugly, and it's not going to be as easy to stick in a github action. If you have a single 'tests' function, then you can do

run: nox --non-interactive --session "tests-${{ matrix.python-version }}(my_dep='${{matrix.my_dep-version}}')" -- --full-trace

now you can already exclude cases from a github action matrix, that's no problem. But the proposed API would allow you to align this with what you get when simply run nox on the command-line

@danieleades
Copy link
Author

I had a quick look at the decorators, I don't imagine it would be too difficult to implement these proposed decorators as essentially filters to the Params list.
Most of the warnings i listed above would be easy enough too, except for warning on overlap 'include' + 'exclude'. That would take some thought. I guess you'd have to thread some metadata through the decorators to track that information

@danieleades
Copy link
Author

danieleades commented Mar 3, 2021

I've got a working draft of these decorators. Would like to know if

  1. this is of interest
  2. what the desired API would look like

Before investing too much more time

@smarie
Copy link
Contributor

smarie commented Mar 4, 2021

I think that the most convenient way would be to provide a "build matrix" dictionary, where

  • the key is the session final id
  • the value is a dictionary containing at least a "python=xx" entry, and the rest of the dictionary should correspond to the parametrized arguments.

For example I have a current code working with :

ENVS = {
    # python 2.7
    "py27-pytest2.x": {"python": PY27, "deps_dct": {"pip": ">19", "pytest": "<3", "pytest-asyncio": DONT_INSTALL}},
    "py27-pytest3.x": {"python": PY27, "deps_dct": {"pip": ">19", "pytest": "<4"}},
    "py27-pytest4.x": {"python": PY27, "deps_dct": {"pip": ">19", "pytest": "<5"}},
    # python 3.5
    "py35-pytest2.x": {"python": PY35, "deps_dct": {"pip": ">19", "pytest": "<3", "pytest-asyncio": DONT_INSTALL}},
    "py35-pytest3.x": {"python": PY35, "deps_dct": {"pip": ">19", "pytest": "<4"}},
    "py35-pytest4.x": {"python": PY35, "deps_dct": {"pip": ">19", "pytest": "<5"}},
    "py35-pytest5.x": {"python": PY35, "deps_dct": {"pip": ">19", "pytest": "<6"}},
    "py35-pytest-latest": {"python": PY35, "deps_dct": {"pip": ">19", "pytest": ""}},
    # python 3.6
    "py36-pytest2.x": {"python": PY36, "deps_dct": {"pip": ">19", "pytest": "<3", "pytest-asyncio": DONT_INSTALL}},
    "py36-pytest3.x": {"python": PY36, "deps_dct": {"pip": ">19", "pytest": "<4"}},
    "py36-pytest4.x": {"python": PY36, "deps_dct": {"pip": ">19", "pytest": "<5"}},
    "py36-pytest5.x": {"python": PY36, "deps_dct": {"pip": ">19", "pytest": "<6"}},
    "py36-pytest-latest": {"python": PY36, "deps_dct": {"pip": ">19", "pytest": ""}},
    # python 3.7
    "py37-pytest2.x": {"python": PY37, "deps_dct": {"pip": ">19", "pytest": "<3", "pytest-asyncio": DONT_INSTALL}},
    "py37-pytest3.x": {"python": PY37, "deps_dct": {"pip": ">19", "pytest": "<4"}},
    "py37-pytest4.x": {"python": PY37, "deps_dct": {"pip": ">19", "pytest": "<5"}},
    "py37-pytest5.x": {"python": PY37, "deps_dct": {"pip": ">19", "pytest": "<6"}},
    "py37-pytest-latest": {"python": PY37, "deps_dct": {"pip": ">19", "pytest": ""}},
    # python 3.8
    "py38-pytest2.x": {"python": PY38, "deps_dct": {"pip": ">19", "pytest": "<3", "pytest-asyncio": DONT_INSTALL}},
    "py38-pytest3.x": {"python": PY38, "deps_dct": {"pip": ">19", "pytest": "<4"}},
    "py38-pytest4.x": {"python": PY38, "deps_dct": {"pip": ">19", "pytest": "<5"}},
    "py38-pytest5.x": {"python": PY38, "deps_dct": {"pip": ">19", "pytest": "<6"}},
    "py38-pytest-latest": {"python": PY38, "deps_dct": {"pip": ">19", "pytest": ""}}
}

As you can see, I use a parameter deps_dct here.

@cjolowicz
Copy link
Collaborator

I'd just like to note that nox.parametrize is already expressive enough to handle a sparse build matrix:

import nox


@nox.session
@nox.parametrize(
    "python,my_dep",
    [
        (python, my_dep)
        for python in ("3.6", "3.7", "3.8")
        for my_dep in ("1.0", "2.0")
        if (python, my_dep) != ("3.6", "2.0")
    ],
)
def tests(session, python, my_dep):
    print(f"{python =} {my_dep = }")

So the issue is not expressivity. It's the fact that nox.parametrize cannot set session attributes such as the Python interpreter.

I'm not proposing to start treating parameters named "python" in a special way. To start with, that would not be backward compatible. Would it make sense though to add a keyword argument to @nox.parametrize that lists parameters that should be passed to the session decorator instead of the session function?

@theacodes
Copy link
Collaborator

However we approach this, the API should match pytest as much as possible.

For example pytest.param is used to set meta attribute such as the test ID.

Nox has implicit parameterization on the Python version, so it might be useful to make that explicit somehow.

@smarie
Copy link
Contributor

smarie commented Mar 4, 2021

Would it make sense though to add a keyword argument to @nox.parametrize that lists parameters that should be passed to the session decorator instead of the session function?

I like this idea !

@cjolowicz
Copy link
Collaborator

To make this a bit more concrete, these two would be equivalent:

@nox.session
@nox.parametrize("python", ["3.6", "3.7", "3.8"], sessionparams=["python"])
def tests(session):
    pass

@nox.session(python=["3.6", "3.7", "3.8"])
def tests(session):
    pass

Not sure if sessionparams is useful for anything but the Python version at the moment.

However we approach this, the API should match pytest as much as possible.

Agreed. Is there some primitive in the pytest API that assigns metadata to the params themselves? IIUC, pytest.param applies to the individual values passed through the params.

@smarie
Copy link
Contributor

smarie commented Mar 5, 2021

Is there some primitive in the pytest API that assigns metadata to the params themselves?

Not that I know. Not sure that you can do much better than your proposal here :)

Not sure if sessionparams is useful for anything but the Python version at the moment.

It would be great to support the following use cases related to the session documentation (currently copied from the function docstring, I do not think it can be overridden with session(doc=) or similar):

  • modify the session documentation explicitly thanks to a doc arg in session (that would be propagated from the parametrize through sessionparams)

  • accept that the docstring is written with the newstyle formatting "foo {bar} foo" so as to generate the associated session documentation for each position in the grid by calling func.__doc__.format(**params).

By the way, the default value of sessionparams could be None and mean : "all known session kwargs that are present in this list" (python, and doc if the above is implemented) so that users do not need to specify it explicitly in most situations.

@cjolowicz
Copy link
Collaborator

@theacodes Should we give this a go?

@danieleades How do you feel about the approach of allowing nox.parametrize to set the Python interpreter? Since you've already put work into this, would you like to take a stab at implementing it (if @theacodes is happy with it)? Give a shout if you'd like any pointers :)

@smarie

modify the session documentation explicitly thanks to a doc arg in session (that would be propagated from the parametrize through sessionparams)

accept that the docstring is written with the newstyle formatting "foo {bar} foo" so as to generate the associated session documentation for each position in the grid by calling func.doc.format(**params).

Currently, nox -l prints the session signature followed by the first line of the docstring. The latter is the same for every parametrization (and every Python). I also feel that this could be improved.

The problem I see with a str.format approach is that it's still quite rigid, and the output already includes parameters in the session names (they're needed for -k). So I'm worried that the session listing would get even more repetitive. Printing descriptions only once for each session group, possibly combined with using the ids keyword, seems to do more to improve readability. Or am I missing something here?

By the way, the default value of sessionparams could be None and mean : "all known session kwargs that are present in this list" (python, and doc if the above is implemented) so that users do not need to specify it explicitly in most situations.

The thing is that users might be using these names for ordinary parameters. We could go a path of deprecation here, of course. Personally, I kind of like the fact that all parameter names are created equal unless explicitly mentioned in sessionparams.

By the way, a different approach that does not have this problem would be to use something other than strings for special parameters, e.g. nox.params.python which could be some kind of enum. But that does not play well with the fact that parameter names are listed as a comma-separated string. So I think that kind of rules this approach out.

@theacodes
Copy link
Collaborator

theacodes commented Mar 5, 2021 via email

@cjolowicz
Copy link
Collaborator

I decided to have a go at this in #404

@cjolowicz
Copy link
Collaborator

It occurred to me that we don't need sessionparams for backward compatibility, after all. We can just inspect the signature of the session function to see if it declares python as an argument. If it does, we'll treat python as a normal parameter. If it doesn't, we use it to determine the session Python.

This simplifies both the public API and the implementation significantly 🎉

#413 implements this approach. I closed #404, but I'm happy to reopen it if people prefer the more explicit approach listing the python parameter in sessionparams.

@cjolowicz
Copy link
Collaborator

Just to recap the API as it stands now... The following two would be equivalent:

@nox.session
@nox.parametrize("python", ["3.6", "3.7", "3.8"])
def tests(session):
    pass

@nox.session(python=["3.6", "3.7", "3.8"])
def tests(session):
    pass

So we no longer need an extra keyword argument sessionparams=["python"] in the parametrize call.

@smarie
Copy link
Contributor

smarie commented Mar 31, 2021

I like this. With the proper level of documentation about what you just mentioned ("if python is an argument of the session function it will be used as a parameter") then it is understandable.

What about other session parameters, would it make sense to have them follow the same principle ?
(reuse_venv, name, venv_backend, venv_params). At least for name this seems redundant with ids ..

@cjolowicz
Copy link
Collaborator

I like this. With the proper level of documentation about what you just mentioned ("if python is an argument of the session function it will be used as a parameter") then it is understandable.

I was going to omit this from the documentation, actually. While we shouldn't break anybody who is using "python" as a normal parameter, I also don't think we should encourage this usage. I have no idea how many people are doing this. If this is a thing, then yes, we should probably mention that it'll continue to work (but discourage it?).

What about other session parameters, would it make sense to have them follow the same principle ?
(reuse_venv, name, venv_backend, venv_params). At least for name this seems redundant with ids ..

Yes, I think that would work in principle. Do you see a use case for parametrizing any of these?

@smarie
Copy link
Contributor

smarie commented Apr 2, 2021

I was going to omit this from the documentation, actually.

I would recommend that all "features" are documented. Otherwise people end up thinking that the lib is not able to do this, because they just don't know what the "happy few" know :)

Do you see a use case for parametrizing any of these?

For the "name" yes, I would be able to give a custom name to each of my sessions when several argnames are parametrized, rather than trying to customize the "ids" function so that it works. This is a problem in pytest as well: you can not specify a unique id for a combination of parametrized argnames, as the ids param applies to each independetly rather than acts as a replacement for the whole group.

For the venv_backend and other venv related params, I guess that using it in parameters will allow people to test that something works both using conda, venv and virtualenv. Maybe edge users, but if this does not cost anything more than parametrizong the python then maybe worth it

smarie pushed a commit to smarie/python-makefun that referenced this issue Jul 4, 2024
…etrization as in pytest (wntrblm/nox#392). Simplified `nox_utils`. Fixes #105
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants