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

sessions: require a session? #453

Open
henryiii opened this issue Jun 14, 2021 · 10 comments · May be fixed by #631
Open

sessions: require a session? #453

henryiii opened this issue Jun 14, 2021 · 10 comments · May be fixed by #631

Comments

@henryiii
Copy link
Collaborator

How would this feature be useful?

Often, especially with building things like binary extensions (which is where I do a lot of work), it is useful to build once and then use the built products (wheels usually, could also be docs) in other sessions. From looking around, it seems like tools like pip currently are just manually running a function that sets up the required produces, and putting that in front of everything that needs it - which is really slow if this is trying to build something.

In the noxfile I'm working on for CMake currently, I'd like to produce the wheel (several minutes) then run an array of tests (all versions of Python) on it, since it doesn't get tied to the current version of Python. But I've also needed this for a "serve" job for docs depending on a "build" job, for different jobs in packages that depend on building a wheel (even if it's only for the current python version), etc.

Describe the solution you'd like

I feel like this is exactly the opposite of notify (or whatever it gets renamed to if #398 changes anything rather than just updating the docs). It could be requires=[...], which would list session that have to run to completion before this session starts. So requires=[build] would not start this session until the build session completes, and would add "build" to the queue if it is not explicitly requested.

A caching system would be amazing, but is orthogonal and not needed to get a benefit for this.

Describe alternatives you've considered

Maybe there's a way to do this that I'm just missing? Happy if there is already a way to do this that I've missed. It can be partially be done by hand, by writing a function that builds an output the first time it's called, and then just does nothing on each subsequent call.

@rmorshea
Copy link
Contributor

As a short-term work around, it's possible to pass an existing session to your session functions:

@nox.session
def parent(session):
    require_1(session)
    require_2(session)
    ...
    dependent(session)

@nox.session
def require_1(session): ...

@nox.session
def require_2(session): ...

...

@nox.session
def dependent(session): ...

This will work so long as your child sessions don't require unique Session.posargs.

@henryiii
Copy link
Collaborator Author

henryiii commented Jun 14, 2021

Will this deduplicate the slow step, though? I'd like:

@nox.session
def build(session):
    ... # slow

@nox.session(require=[build])
def test1(session):
    ...

@nox.session(require=[build])
def test2(session):
    ...

To only build once. I can cache on the build manually, though ("has_run = False, global has_run, has_run=True").

has_built = False

@nox.session
def build(session):
    global has_built
    if not has_built:
        ... slow
        has_built = True

@nox.session
def test1(session):
    build(session)
    ...

@nox.session(require=[build])
def test2(session):
    build(session)
    ...

Or does it do the deduplication already?

@rmorshea
Copy link
Contributor

rmorshea commented Jun 14, 2021

I see. No it does not deduplicate.

I'm also realizing that instead of calling the functions directly to stop on an error, you could just set --stop-on-first-error when running the session.

Given all that, what if you organized your tasks like this where you would run nox -s test --stop-on-first-error?

@nox.session
def test(session):
    session.notify(build)
    session.notify(test1)
    ...
    session.notify(testN)

@nox.session
def build(...): ...

@nox.session
def test1(...): ...

@nox.session
def testN(...): ...

If you commonly found that you only wanted to run a subset of tests you could do:

@nox.session
def test(session):
    session.notify(build)
    for test_name in session.posargs:
        session.notify(test_name, posargs=[])

With usage like nox -s test --stop-on-first-error -- test1 ... testN

Given this common re-use of the --stop-on-first-error perhaps if could be useful to set that in the session definition (e.g. nox.session(stop_on_first_error=True))

@henryiii
Copy link
Collaborator Author

Notify is backwards from what is useful for this sort of situation. I want to say nox -s test1 and that will build, then run test1. Or I want to say nox -s test2,test3 and then build once, then run test2, then test3. An error in build should cause the sessions that depend on it to not run, but otherwise, they should each run (unless --stop-on-first-error is set). I don't necessarily have multiple tests, but I have tests, docs, and build, and would really like build to always run once, with tests and docs always coming after it.

For the workaround above, a user will see all the above sessions, including test1, and won't know those can't be run without running build first or without using the special "test" session. Or that they should have to add --stop-on-first-error, or the sessions as arguments (could be suggested to them with #454 though? :) )

Is adding a requires to handle this a good idea? And is that a good design for that idea?

@cjolowicz
Copy link
Collaborator

Which generator are you using with CMake? Shouldn't that already take care of incremental builds, even across multiple Nox runs?

@rmorshea
Copy link
Contributor

rmorshea commented Jun 15, 2021

I think that to really implement something like require correctly, Nox would need some concept of a DAG of tasks it needs to complete which, to my knowledge, it does not have, so I'm not sure it really makes sense to add.

Given that, I still think you can accomplish what you're looking for by not making test_1 ... test_n sessions and instead passing them directly into Session.notify as normal functions. Then, with the addition of a stop_on_first_error parameter to the session decorator I think you could get the same effect:

@nox.session(stop_on_first_error=True)
def test(session):
    session.notify(build)
    for test_name in session.posargs:
        test_func = globals()["test_" + test_name]
        session.notify(test_func)

def test_1(session): ...
...
def test_n(session): ...

You're usage then could simply be nox -s test -- 1 ... n with the added benefit that test_1 ... test_n are not publicly exposed and can only be run through the root test session.

One could certainly argue that the use of globals() is a bit untoward, but it's fairly simple, and could be made even more so, by the addition of "hidden" sessions. That is, sessions that can be referenced by name in Session.notify but which do not show up publicly in the list of available sessions to run at the CLI.

@FollowTheProcess
Copy link
Collaborator

This would be a cool feature!

It's not nox but I've used doit for these types of things before and it works quite well. My only complaint is that it's not as nice as nox when printing (no colours in things like pytest output for example).

@smarie
Copy link
Contributor

smarie commented Nov 8, 2021

See also #167, maybe this is a duplicate ?

@henryiii
Copy link
Collaborator Author

henryiii commented Feb 4, 2022

Nox would need some concept of a DAG of tasks it needs to complete which, to my knowledge, it does not have

Python 3.9's topological sorter (or a backport package) sounds really useful for something like this! See https://www.youtube.com/watch?v=_O9q3H6mocg for a nice overview.

@gschaffner gschaffner linked a pull request Jul 13, 2022 that will close this issue
@gschaffner
Copy link

Hi all! There is an implementation of this over on #631. It would be great to get some feedback there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

6 participants