Skip to content

Conversation

@erikkemperman
Copy link
Contributor

@erikkemperman erikkemperman commented May 30, 2022

Description

There is a minor block formatting error in docs/source/command_line.rst, as committed in a6166b2:

The default logic used to scan through search paths to resolve imports has a
quadratic worse-case behavior in some cases, which is for instance triggered
by a large number of folders sharing a top-level namespace as in:
foo/
company/
foo/
a.py
bar/
company/
bar/
b.py
baz/
company/
baz/
c.py
...
If you are in this situation, you can enable an experimental fast path by
setting the :option:`--fast-module-lookup` option.

I noticed this during a local tox build, and then wondered why this wasn't flagged in CI.

It turns out that the docs github workflow wants to run the build on Python 3.10, as of commit fec5320:

env:
TOXENV: docs
steps:
- uses: actions/checkout@v2
- uses: actions/setup-python@v2
with:
python-version: '3.10'

This fails, however, because tox.ini specifies Python 3.7 for the docs build:

mypy/tox.ini

Lines 56 to 58 in 2051024

[testenv:docs]
description = invoke sphinx-build to build the HTML docs
basepython = python3.7

Subsequently, though, this failure is ignored, and the CI passes, because skip_missing_interpreters is enabled:

mypy/tox.ini

Line 3 in 2051024

skip_missing_interpreters = true

Test Plan

With these changes, the docs workflow gets Python 3.7, and the docs are actually built successfully. I'm not sure why the author of the theme change thought it had to be 3.10...

@erikkemperman
Copy link
Contributor Author

Whilst I was tracking this down, I ran into a bunch of redundancies and inconsistencies. For example, the list of environments given in tox.ini seems out of date relative to setup.py and the workflow matrix. The versions listed in pyproject.toml don't align with the various requirement files, and while the latter versions do line up well will the workflows, this is only because of manual edits having been done consistently in several places. It would be more robust, it seems to me, to have a single source of truth for these things whenever possible.

Also, it seems to me that settings such as skip_missing_interpreters and isolated_build make sense for local builds, but perhaps they should be overridden in CI builds (where missing interpreter warnings shouldn't be ignored because it will just skip those bits, and because builds are already isolated due to the CI architecture it seems slightly wasteful to have another layer of virtualenv for no good reason).

I also notice that the mypyc docs aren't being tested in CI?

There is some coverage configuration in some places, but I don't think the CI is currently doing anything with it?

Now, I completely understand how these things happen, as repositories grow organically, but I am tempted to try and address some of these "problems". However, I also appreciate that I am brand new around here, and what I am considering would perhaps end up being quite an invasive set of changes... So before I spend a lot of time on this I would like to just check with the maintainers, is this kind of proposal something you would welcome?

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for catching this!

PRs improving CI are welcome. Agreed that skip_missing_interpreters would ideally be overridden in CI. I'm okay with isolated_build being on, maybe it could catch issues if we forget to declare some build dependency or something. Definitely makes sense to check mypyc docs in CI. I'm not sure how much value mypy would currently get much value out of running coverage in CI, but it's nice to be able to run some stuff ad hoc, so maybe don't spent too much time fiddling with that one.

If there are other changes to CI you want to make that you want to run by someone before putting in the time, feel free to tag me! :-)

@hauntsaninja hauntsaninja merged commit 35b4d8a into python:master May 30, 2022
@erikkemperman
Copy link
Contributor Author

@hauntsaninja Well that was super quick, thanks! All right, then, I’ll play around some more and see what I can come up with.

@erikkemperman erikkemperman deleted the fix-commandline-rst-and-ci branch May 30, 2022 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants