Add an option to skip install commands

[emilmelnikov]

How would this feature be useful?
Frequently you just want to re-run a particular session in a reused virtual environment as quickly as possible (e.g for unit tests, docs or linting). This is possible with --reuse-existing-virtualenvs, but session.install() still takes a second or two, which is not a lot, but still slower than just running pytest directly. Therefore, it would be useful to have an option for skipping session.install(), especially when executing sessions locally in a development loop.

Describe the solution you'd like
Add an inverse of the --install-only command.

Describe alternatives you've considered

• Use invoke within a manually created virtual environment (this is what I've been doing).
• Just use a corresponding Python interpreter directly, e.g. .nox/<session_name>/bin/python -m pytest.
• Accept the 1-2 second overhead, and take your time to sip on your favourite beverage :)

[theacodes]

Sounds good to me. Seems like we should add the --no-install flag.

[gwynforthewyn]

I'm interested in seeing if I can figure this out 🙂

[gwynforthewyn]

I added what seemed like the right options change in the branch at https://github.com/PlayTechnique/nox/tree/394-add-no-install-flag

I started getting test failures in tests/test_main.py. They all stem from the same method:

____________________________________________________________________________________________________________________________________________________________________________ test_main_no_args _____________________________________________________________________________________________________________________________________________________________________________

monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x102fbac10>

    def test_main_no_args(monkeypatch):
        monkeypatch.setattr(sys, "argv", [sys.executable])
        with mock.patch("nox.workflow.execute") as execute:
            execute.return_value = 0

            # Call the function.
            with mock.patch.object(sys, "exit") as exit:
>               nox.__main__.main()

tests/test_main.py:51:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
nox/__main__.py:30: in main
    args = _options.options.parse_args()
nox/_option_set.py:267: in parse_args
    self._finalize_args(args)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <nox._option_set.OptionSet object at 0x1026e5a90>, args = Namespace(help=None, version=None, list_sessions=None, sessions=None, pythons=None, keywords=None, posargs=[], verbose...ernal_run=None, install_only=None, no_install=None, report=None, non_interactive=None, nocolor=False, forcecolor=False)

    def _finalize_args(self, args: Namespace) -> None:
        """Does any necessary post-processing on arguments."""
        for option in self.options.values():
            # Handle hidden items.
            if option.hidden and not hasattr(args, option.name):
                setattr(args, option.name, option.default)

>           value = getattr(args, option.name)
E           AttributeError: 'Namespace' object has no attribute 'no_install_only'

nox/_option_set.py:255: AttributeError

I inspected the options in the self.options.values collection and compared the install_only and no_install_only options to another option collection that's built by the make_flag_pair method. The other option contained a value in merge_func that looked something like this:

merge_func : functools.partial(<function flag_pair_merge_func at 0x10d9c9c10>, 'error_on_external_run', 'no_error_on_external_run')

However, for the install_only options this merge_func is set to None. That seems suspect, but it's not clearly related to the error 'Namespace' object has no attribute 'no_install_only'

I do also see that the help doc string for the options has this tantalising tidbit:

Args:\n name (str): The name used to refer to the option in the final namespace\n object.

Do I maybe need to register the 'no_install_only' in this namespace object somehow?

[gwynforthewyn]

Aha! I see! I updated the noxfile to add some additional arguments to pytest

"pytest", "--cov=nox", "-x", "--pdb", "--tb=native", "--cov-config", ".coveragerc", "--cov-report=", *tests

I then ran the nox command from CONTRIBUTING.md and got a couple of additional clues. First, the native stacktrace:

>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> traceback >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
Traceback (most recent call last):
  File "/Users/jams/Developer/nox/tests/test_main.py", line 51, in test_main_no_args
    nox.__main__.main()
  File "/Users/jams/Developer/nox/nox/__main__.py", line 30, in main
    args = _options.options.parse_args()
  File "/Users/jams/Developer/nox/nox/_option_set.py", line 267, in parse_args
    self._finalize_args(args)
  File "/Users/jams/Developer/nox/nox/_option_set.py", line 255, in _finalize_args
    value = getattr(args, option.name)
AttributeError: 'Namespace' object has no attribute 'no_install_only'

Then I tried inspecting the namespace object to see what attributes are there. The Namespace docs seemed to say that the vars() idiom should dump that info:

(Pdb) vars()
{'self': <nox._option_set.OptionSet object at 0x10fb56280>, 'args': Namespace(help=None, version=None, list_sessions=None, sessions=None, pythons=None, keywords=None, posargs=[], verbose=None, add_timestamp=None, default_venv_backend=None, force_venv_backend=None, no_venv=False, reuse_existing_virtualenvs=None, no_reuse_existing_virtualenvs=None, noxfile='noxfile.py', envdir=None, extra_pythons=None, stop_on_first_error=None, no_stop_on_first_error=None, error_on_missing_interpreters=None, no_error_on_missing_interpreters=None, error_on_external_run=None, no_error_on_external_run=None, install_only=None, no_install=None, report=None, non_interactive=None, nocolor=False, forcecolor=False), 'option': <nox._option_set.Option object at 0x10fc472b0>, 'value': None}

I could see that my no_install is there just fine, but it looks like the argument parser is looking for no_install_only. I made the bold assumption that probably there's just an expectation that command line flags are configured in a standard pattern with --foo being expected to be negated by --no-foo. I updated the flag to be --no-install-only, and the tests pass 🙂

[gwynforthewyn]

Next, I just need to work out how to execute nox from inside its directory, so I can verify the option's presented in the help etc and that it's doing something.

[gwynforthewyn]

Oh! I think I misunderstood the original request! It's not just for a negation of --install-only, that may have only been an example of the kind of thinking that emilmelnikov was thinking of.

[cjolowicz]

Some pointers for @jamandbees or anybody interested in tackling this:

• We need an additional option --no-install. As you noticed, this is not the negation of --install-only. It's a separate option that causes install commands to be skipped.
• If the new option is specified, session methods for installing packages should exit early. Currently we have the following methods for package installation: session.install, session.conda_install, session.run_always.

You can take a look at the analogous code for --install-only in session.run for how to implement this logic.

HTH :)

[gwynforthewyn]

That's awesome, thanks @cjolowicz!

[gwynforthewyn]

I have something that covers the session.install and session.conda_install methods specifically - here's the compare URL: https://github.com/theacodes/nox/compare/main...PlayTechnique:394-adds-no-install-flag?expand=1

I wanted to double check that session.run_always should definitely have the early guard, too. The docs for it indicate the intention to always run that method (from sessions.py 275-176):

        This is a variant of :meth:`run` that runs in all cases, including in
        the presence of ``--install-only``.

I'm assuming you're right and that the run_always method should have the guard, but when I looked at the implementation I also wasn't sure what I should have the no-install guard's logger message should log in there. In the other two methods, it was pretty clear that I was saying "these specific packages were skipped", but I'm not familiar enough with --run-always to know quite how the package names are discovered in there.

This is what I had come up with:

        if self._runner.global_config.no_install:
            logger.info("Skipping run_always, as --no-install is set."
            return None

        if not args:
            raise ValueError("At least one argument required to run_always().")

        return self._run(*args, env=env, **kwargs)

Could I get a little extra assistance on this? Thanks 🙂

[gwynforthewyn]

Actually, I'll just implement it like above and create the pull request and we can move this into a PR review. That might work be easier for you folks instead of offering patches in comments 🙂

[gwynforthewyn]

Here's the PR I said I'd pop up #405

[emilmelnikov]

What do you think about adding a shortcut option for --reuse-existing-virtualenvs --no-install? Such an option would be convenient for the interactive use case described above, but, of course, it is not strictly necessary.

[cjolowicz]

Thank you @jamandbees for submitting your PR!

I have some questions about how --no-install should work, so I thought I'll ask them here on the issue before we get into implementation details on your PR.

Maybe @theacodes or the other maintainers would like to chime in here as well.

1.

The first question is about what @emilmelnikov just remarked:

What do you think about adding a shortcut option for --reuse-existing-virtualenvs --no-install? Such an option would be convenient for the interactive use case described above, but, of course, it is not strictly necessary.

It seems to me that --no-install should imply --reuse-existing-virtualenvs. If the previous virtualenv was removed and created afresh every time, then skipping the installation steps would likely lead to errors in later commands. Does that make sense?

2.

The second question is about what we should do if an environment does not exist yet. If the environment was just created from scratch, skipping the installation steps would, again, lead to errors later on. There are some options here:

• Raise an error message informing the user that installation cannot be skipped because the environment does not exist yet.
• Create the environment and run the installation anyway. Possibly warn the user that --no-install was ignored for this environment.
• Anything else?

Personally, I think I prefer the second behavior, as the user probably just wants to speed up repeated runs. Also, this behavior is more in the spirit of --reuse-existing-virtualenvs, which happily creates the environment if it does not exist yet.

The first behavior could make sense if there's some specific other reason against running install commands, like avoiding network access. I'm not sure how relevant this use case is. The first behavior is also more in line with what it says on the tin: --no-install categorically states that installation commands will not be run. (Should the option be named differently then? --skip-install-in-existing-virtualenvs seems overly long, but it does more accurately describe the second behavior.)

3.

My third question is about whether session.run_always should be skipped when --no-install is passed.

The name run_always and the documentation ("runs in all cases") suggest that it should not be skipped.

However, I would argue that run_always should be skipped.

Here are two important use cases for run_always:

• Run build commands that are a prerequisite for pip install
• Install packages using a tool other than pip, e.g. Poetry or pipenv

These are clearly part of the installation phase of the session.

Currently, the only purpose of run_always is to ensure that the command is run in the presence of --install-only. So it's hard to imagine why you'd want to run anything that's not related to installation.

Personally, I have a use case where the session builds a wheel, uninstalls it from the environment using run_always, and then passes it to session.install. This is done to ensure that pip installs the wheel even if the version did not change. This session would break if --no-install were to skip session.install but not the preceding pip uninstall.

(You can tell from what I'm saying above that I'm not a huge fan of the name run_always, as opposed to e.g. session.runinstall or such. But let's keep that discussion out of this issue.)

[emilmelnikov]

When the new hypothetical --skip-install-in-existing-virtualenv flag is passed, from a user's point of view, if an environment does not yet exist, it makes sense just to run the target session as usual, creating a new environment and installing everything, but if the environment already exists, skip install and run_always commands.

[gwynforthewyn]

Hey folks - I just want to be sure to let you know that, whatever the outcome of this chat, I'll be happy to implement the changes.

I appreciate the time you're putting in to make this better 🙂

[cjolowicz]

Thanks @jamandbees, actually the points above don't require many changes. How about if we proceed to iterate on your PR?

You already implemented point 3 (skipping run_always).

Point 1 (implying --reuse-existing-virtualenvs) requires a one-line change: SessionRunner._create_venv checks if the virtualenv should be reused, we'd need to add the new flag to that condition.

Point 2 (ignoring --no-install in new environments) requires two things:

• The virtualenv needs to remember if it was reused. This could be a bool attribute ProcessEnv.reused, initially False. The VirtualEnv and CondaEnv implementations can set this to True in their create methods when the virtualenv gets reused. The logic is already there: These methods call self._clean_location which returns False if the virtualenv was reused.

• The installation methods should log a warning if --no-install was given and session.virtualenv.reused is False, and perform the installation anyway.

As for the option name, let's wait for feedback/additional ideas. Maybe a short option -R (as its related to -r) would make the long explicit name more acceptable?

[gwynforthewyn]

Sounds good! I'll take a look today ☺️ Thank you!

[cjolowicz]

Here's a different proposal:

• Keep the name --no-install, as originally proposed.
• Revert the decision that --no-install implies reuse of virtualenvs. Instead, make the option have no effect unless --reuse-existing-virtualenvs is also given.

@jamandbees Would you be okay with doing this change? Specifically, in SessionRunner._create_venv, we'd need to remove --no-install from the check if the virtualenv should be reused. (And that's all that's needed.)

We also may want to revisit when and how warnings are printed. Currently there is a warning every time an installation happens despite --no-install. It feels sufficient to only warn when --no-install is specified without --reuse-existing-virtualenvs (because this would never have any effect).

[gwynforthewyn]

@cjolowicz I'm super happy to implement whatever changes you folks decide upon! I'm just stoked you all are hanging out and giving me direction 🙂

Would you like me to implement the above suggestion on a different branch, so you can see how it looks?

[cjolowicz]

@jamandbees You're welcome :) Thanks for implementing it!

Would you like me to implement the above suggestion on a different branch, so you can see how it looks?

You can just continue on the existing PR. It also makes it easier for us to keep track of things 😉

[emilmelnikov]

• Keep the name --no-install, as originally proposed.
• Revert the decision that --no-install implies reuse of virtualenvs. Instead, make the option have no effect unless --reuse-existing-virtualenvs is also given.

If this will be implemented, then I would like to ask for a shortcut option that combines --no-install and --reuse-existing-virtualenvs because this is what most people will use.

[gwynforthewyn]

How does —reuse-all sound for that option?

I’ve been trying to work on nox over the weekend but I got my covid vaccine last week and was pretty wiped out for a few days. I’m hoping to knock this out this week 🙂

[gwynforthewyn]

I've implemented the request; I've decreased test coverage, though, and am just trying to figure that out.

[gwynforthewyn]

I've been poking at this test coverage issue on and off all day and made no headway. It seems to me like the coverage tool is saying that lines which are explicitly verified are not being tested. The coverage report shows nox/sessions.py 269 0 94 2 99% 382->388, 454->459, but lines 382-388 are definitely verified and so are 454-459. At least, I think so!

I went ahead and pushed the change; is there any chance you could take a look when you have a minute @cjolowicz and see if you can see something I don't? 🙏

Beyond that, I could see how to add a new option that implies both --no-install and ``--reuse-existing`, it looks something like this:

    _option_set.Option(
        "reuse_venv_no_install",
        "--reuse-venv-no-install",
        default=False,
        group=options.groups["secondary"],
        action="store_true",
        help="Equivalent to --reuse-existing-virtualenvs --no-install.",
    ),

However, I want to implement that by flipping reuse_existing_virtualenvs and no_install to true, and I'm just figuring out where in the codebase to do that.

[gwynforthewyn]

Oh! I have an idea for that second problem! Lemme work it out 🙂

[gwynforthewyn]

Hey gang - I'm still on this, I'm just also busy with some other stuff that will hopefully mellow out in the next week.

Just to write this down, my idea for working on the option requested above is that I don't think there's currently a way to specify an option on the command line which only has the side-effect of setting other options to a known state, so I was thinking of implementing a new option type that allowed for that behaviour. I had read over the _option_sets.py code and the _options.py code; I'm pretty sure I can implement a new option type, but if you can think of a simpler solution then I'd be happy to hear it 🙂

[gwynforthewyn]

Again, sorry for dropping this for a bit! I was studying/interviewing for jobs, but I'm all done with that now. I'm looking forwards to finishing this up.

[cjolowicz]

@emilmelnikov @purefunctor This is being addressed in #432, feel free to comment.