Change default behavior for --error-on-missing-interpreters

[broady]

Is it possible to improve --error-on-missing-interpreters with "principle of least surprise" in mind? We don't want onerous failures during local dev, but the two changes are proposed:

• Mention the --error-on-missing-interpreters flag in the error / warning / success message if an interpreter was skipped
• Change --error-on-missing-interpreters to default ON (instead of OFF) if we've detected the code is in CI (CI=true) or in an automated environment (sys.stdout.isatty()==False)

---

Original Content (Original title: Test suite skipped with error "Python interpreter 3.8 not found.)

Describe the bug

The desired Python version was not available, the test suite is skipped entirely.

2020-08-21T17:46:46.1946078Z ##[group]Run nox
2020-08-21T17:46:46.1946291Z �[36;1mnox�[0m
2020-08-21T17:46:46.1990422Z shell: /bin/bash -e {0}
2020-08-21T17:46:46.1990565Z env:
2020-08-21T17:46:46.1990705Z   pythonLocation: /opt/hostedtoolcache/Python/3.7.8/x64
2020-08-21T17:46:46.1990822Z ##[endgroup]
2020-08-21T17:46:46.2954365Z Running session tests-3.8
2020-08-21T17:46:46.3002157Z Session tests-3.8 skipped: Python interpreter 3.8 not found.

How to reproduce

import nox

@nox.session(python=["3.7"])
def tests(session):
    """Run pytest suite"""
    session.create_tmp()
    session.install("-r", "requirements-test.txt")
    tests = session.posargs or ["tests/"]
    session.run("pytest", *tests)

# TODO: add lint, type checking, etc.

$ cat .github/workflows/data_prep_tools_test.yml 
# This workflow will install Python dependencies, run tests.
# For more information see: https://help.github.com/actions/language-and-framework-guides/using-python-with-github-actions

name: test

on:
  push:
    branches: [ master ]
  pull_request:
    branches: [ master ]
    
defaults:
  run:
    working-directory: tools

jobs:
  build:

    runs-on: ubuntu-latest
    strategy:
      matrix:
        python-version: [3.7]

    steps:
    - uses: actions/checkout@v2
    - name: Set up Python ${{ matrix.python-version }}
      uses: actions/setup-python@v2
      with:
        python-version: ${{ matrix.python-version }}
    - name: Install dependencies
      run: |
        pip install nox
    - name: Test with nox
      run: |
        nox

Expected behavior

non-zero exit code

[theacodes]

This is intentional behavior and you can fail these sessions using --error-on-missing-interpreters - see https://nox.thea.codes/en/stable/usage.html#failing-sessions-when-the-interpreter-is-missing

[broady]

Thanks! As a user, this was unexpected (CI passing when no tests were run)

To minimize surprise, I would suggest:

• make this a hard failure when no interpreter in the matrix is found (i.e., no tests were run)
• mention the --error-on-missing-interpreters flag in the error/warning/success message if an interpreter was skipped

Are those reasonable or do I misunderstand the intent?

[dhermes]

• mention the --error-on-missing-interpreters flag in the error/warning/success message if an interpreter was skipped

I think this is perfectly reasonable and we should do it. That said, I think in CI you still would have the same "I didn't notice that" experience.

[theacodes]

Two is definitely reasonable, but I'm hesitant to do one.

…

On Fri, Aug 21, 2020 at 2:29 PM Chris Broadfoot ***@***.***> wrote: Thanks! As a user, this was unexpected. To minimize surprise, I would suggest: - make this a hard failure when no interpreter in the matrix is found (i.e., no tests were run) - mention the --error-on-missing-interpreters flag in the error/warning/success message if an interpreter was skipped Are those reasonable or do I misunderstand the intent? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#340 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB5I465JOED2GTRIJYBSJDSB24HVANCNFSM4QHQTSYA> .

[dhermes]

@theacodes How would you feel about changing the default value of --error-on-missing-interpreters depending on environment? Things we could do to decide to make it default ON (vs. default OFF):

• The CI=true environment variable is set (Travis CI, CircleCI, AppVeyor and GitHub Actions all set this, AWS CodeBuild does not)
• sys.stdout.isatty() is False
• sys.stdin.isatty() is False

[theacodes]

Hmmm I don't like the idea of differing default behavior based on environment.

…

On Fri, Aug 21, 2020, 2:40 PM Danny Hermes ***@***.***> wrote: @theacodes <https://github.com/theacodes> How would you feel about changing the default value of --error-on-missing-interpreters depending on environment? Things we could do to decide to make it default on: - CI=true is set (I'm pretty sure TravisCI, CircleCI and AppVeyor all set this, not sure about GitHub actions or the AWS code builder thing) - sys.stdout.isatty() is False - sys.stdin.isatty() is False — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#340 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB5I4YHEHZ3KOS7YHFYFX3SB25THANCNFSM4QHQTSYA> .

[dhermes]

Hmmm I don't like the idea of differing default behavior based on environment.

I am with you on that one in pretty much all cases. But it isn't a great experience (and it's fairly common) when someone is running in CI like @broady reported here. I feel like switching --error-on-missing-interpreters to default on makes local dev a bit painful too.

[theacodes]

Alright, coming back to this I'd be willing to review a PR that:

1. Prints out a warning when interpreters are skipped saying that you might want to use --error-on-missing-interpreters and that this will fail by default on CI systems.
2. Makes --error-on-missing-interpreters default to on for non-interactive sessions.

[henryiii]

GitHub Actions recently removed some old Python versions from the default installs, which caused the nox action to silently no longer include them - but my jobs were "passing" and I had no idea that I was not running anything, even though I was explicitly running one job per line. nox -s tests-3.6 was passing even though nothing was found. Once I fixed this, I realized I had not never been running PyP y on Windows, because that was always empty (different name on windows, haven't figured out how to fix that yet). I'm now realizing that all my CI jobs everywhere need a --error-on-missing-interpreter flag on every invocation, or my CI is useless, it will pass even if no Python is found. Before seeing this discussion, I had several alternative ideas (not to all be implemented):

1. Make it an error if no interpreter at all is found - at least one must match. nox -s tests-3.6 should fail if there is no Python 3.6. nox -s tests should fail if there are no Python interpreters matching any of the parametrized options - which is very rare usually, since you have at least the Python you are running Nox on. nox -s lint should fail if you require Python 3.8 for that job and it's not available. At least one job should run to "pass".
2. Make not finding a Python interpreter return an error code, even if it displays something non-error-like. Interactively, you are not really doing much with error codes, that's usually in scripting or CIs. That would retain the nice user behavior but enhance the scripting behavior.
3. Make CI=true change the default error-on-missing-interpreters to True (that one is listed above) This is a really nice convention and not specific to a single CI vendor.
4. Add an environment variable way to set this so I don't have to pass --error-on-missing-interpreters to every single nox run.

On reading the above, I'm not sure I like changing the "non-interactive sessions" behavior, it's really tricky to get non-interactive sessions right - piping to a pager, for example.

Since I always run one Python version per action or job, my option (1) would be fine with me, even though it could probably be implemented in parallel with another option. It would not break local running 99% of the time, since you are either running on all Python versions and at least one matches (the one you are on), or you ask for a specific version and it's quite fine to error if it's not found.

Also, by "error" I mean produce an error code. I don't really care if it's exactly the same printout as we have today. I just need to know to look in my CI logs!

[henryiii]

Also, quick workaround that doesn't seem to be mentioned yet, but can be implemented at the noxfile level:

if os.environ.get("CI", None):
    nox.options.error_on_missing_interpreters = True

I'd rather not have to add that to dozens of noxfiles, and have to include that in docs and tutorials, but that's a way to improve CI safety for now.

[henryiii]

@theacodes @dhermes @FollowTheProcess @cjolowicz (and anyone else) thoughts here? I would really, really like the error code if at least one matching interpreter is not found (I'm totally fine with exactly the same text printout, I just want the error code so CI doesn't "pass"). And maybe some of the other ideas. But I (almost) always run and recommend one Python process per step or job, so that would fix CI in a lot of cases.

Scikit-build is an example where several "passing" jobs were passing, with the only clue being the job length of a few seconds.

[FollowTheProcess]

I've been tripped up by this too, and actually so has Nox itself: #475.

IMO the average user shouldn't have to care about this which means we can't add pain to local runs whilst doing the correct thing on CI, I think the cleanest way of doing this is checking for the CI env variable and making sure local skipped sessions print out a nice helpful warning saying they will not skip on CI and will fail instead if the interpreter is not found as @theacodes suggests.

Maybe say if you want to emulate behaviour on CI, set CI in your environment or run with --error-on-missing-intepreters.

I appreciate the concern that this is different behaviour based on a somewhat hidden environment state but ultimately, CI is a different environment and in this case I think it makes sense that we behave differently.

I'm also not against the --non-interactive based solution, I appreciate @henryiii's concern about paging but I suspect that not a huge amount of people are piping their nox runs to a pager? I could be wrong though!

Either way, something we could do quickly to address this is document @henryiii's workaround:

if os.environ.get("CI", None):
    nox.options.error_on_missing_interpreters = True

We could discuss forever, I think the best thing to do is just take a crack at a solution and get it into PR review for us all to discuss a bit more concretely. I'll try and have a go over the next week or so 👍🏻

[henryiii]

IMO, error codes no not cause the average user to "care". You don't normally even see them on the command line, they do not cause a failure in scripts unless you set an option, etc. Error codes are using when scripting with things like && / ||, or in CI. I don't think nox's friendly printout should change. I just think there's at least one case were it should return an error code other than 0.

If you run nox and request a session, and no session runs because no matching interpreter is found, that should return an error code. The printout doesn't have to even change, but it could - nox should run at least one thing before saying "completed successfully" if you ask for a session. nox -s lint should not complete successfully if it does absolutely nothing - you haven't linted anything! nox -s tests only running 1-2 interpreters still means you are at least no broken on those interpreters, so it's fine to "succeed". Also fine if vanilla nox always succeeds, I just think requesting a session should fail if no interpreters at all are found for that session.

I've worked with that part of the code before, pretty sure I could provide a fix if you are agreeable.

I can't actually reproduce this locally for an illustration, because unfound interpreters are always errors for me locally, due to #381 🤦. This never has bothered me enough to care, though, since I just ignore the error message and look at the ones that do complete (or ask for the one I want to run that's not in pyenv).

[henryiii]

I'm not "against" the non-interactive solution either, I just know from experience with color that it's a bit finicky. And interactively, error codes don't matter anyway, so I don't really see a point in toggling error codes based on interactivity. CI or always having them on seems more controllable and reliable, that's all.

[FollowTheProcess]

IMO, error codes no not cause the average user to "care".

Sorry if I wasn't clear, I wasn't referring to exit codes specifically as impacting users, more so any solution that would cause skipped local missing interpreter sessions to now cause an error for example defaulting --error-on-missing-interpreters to True. Or anything that would require them to configure their noxfile in a certain way etc. I didn't want anyone who's skipping sessions locally now to suddenly start failing is what I meant.

If you run nox and request a session, and no session runs because no matching interpreter is found, that should return an error code.

Sorry if I'm not understanding you, are you suggesting this as behaviour in all situations (i.e. both on CI and local dev) or just to be applied when in CI?

I think we're broadly on the same page. If we check for CI and if it's present, effectively set --error-on-missing-interpreters to True internally for that run, this would fail the build and solve this issue right?

[cjolowicz]

+1 for option 3:

3. Make CI=true change the default error-on-missing-interpreters to True (that one is listed above) This is a really nice convention and not specific to a single CI vendor.

Additionally, it would be good to add the warning described here:

1. Prints out a warning when interpreters are skipped saying that you might want to use --error-on-missing-interpreters and that this will fail by default on CI systems.

I'm -0.5 for option 1:

1. Make it an error if no interpreter at all is found - at least one must match.

• it's a larger breaking change as it affects local runs
• it means that --[no-]error-on-missing-interpreters is no longer the single source of truth for what happens if an interpreter is not found.
• option 3 is simpler to implement and solves the use case just as well, if not better
• there are tricky edge cases: session.notify, @nox.parametrize with a matrix including Python versions, and generally the many ways of specifying interpreters and sessions

[henryiii]

Okay, mostly really looking forward to it being solved! If I have a chance to start a PR, I'll drop a note here (unless it's really quick).