Fix CI #800

Jasha10 · 2021-10-12T22:38:09Z

There is a CI failure in #799.
I want to see if CI fails on the master branch.

Jasha10 · 2021-10-12T23:10:38Z

It seems sudo pip install nox is succeeding but then nox --add-timestamp fails:

ERROR: Could not install packages due to an OSError: [Errno 2] No such file or directory: '/home/circleci/project/.nox/omegaconf-3-9/bin/python'

omry · 2021-10-13T08:45:37Z

No need for an empty commit to rerun CI:

jieru-hu · 2021-10-14T15:23:26Z

@Jasha10 you can also run the test on a CircleCI instance for debugging. let me know if you need more info on this, happy to chat offline.

Jasha10 · 2021-10-14T16:34:46Z

Thanks for the tip, @jieru-hu! I managed to get ssh working with a running CircleCI instance.
I can confirm that the file /home/circleci/project/.nox/omegaconf-3-9/bin/python does not exist on the instance. I'm looking to see if there are some nox best-practices that we could be following...

jieru-hu · 2021-10-14T17:50:38Z

nice, could you add more context on the fix?

Jasha10 · 2021-10-14T18:04:30Z

I'm not sure why exactly session.install("pip") was causing problems with finding the python executable.

The trouble was happening with this function from the noxfile:

def deps(session, editable_installl):
    session.install("--upgrade", "setuptools", "pip")
    extra_flags = ["-e"] if editable_installl else []
    session.install("-r", "requirements/dev.txt", *extra_flags, ".", silent=True)

If the first call to session.install includes "pip", that would cause the second call to session.install (i.e. the installation of "requirements/dev.txt" and ".") to fail with an error that .nox/omegaconf-3-9/bin/python could not be found.

My guess is that the session.install command is using pip to perform the installation, so maybe there is confusion if pip is upgraded within the same session.

Anyway, the examples on the nox docs do not suggest that using session.install to upgrade "pip" is necessary.

Jasha10 · 2021-10-14T18:05:35Z

On a related note:
I suspect that session.install("setuptools") might also be unnecessary.

jieru-hu · 2021-10-14T18:17:25Z

it's probably still a good idea to upgrade pip. in Hydra's nox, session.run is used instead of session install, do you mind giving that a try instead? https://github.com/facebookresearch/hydra/blob/main/noxfile.py#L186-L188

Jasha10 · 2021-10-14T19:12:45Z

Commit 9adc80d fails with the following nox commands:

session.run("python", "-m", "pip", "install", "--upgrade", "setuptools", "pip")
session.run("pip", "install", "-r", "requirements/dev.txt", ".", silent=True)

It's the same error as before: the python executable cannot be found.

Interestingly, I'm getting slightly different error messages when I try to reproduce the failure in an ssh session.

it's probably still a good idea to upgrade pip.

In the most recent commit 134be30 I've modified the .circleci/config.yml file to upgrade pip before the nox session starts.

jieru-hu · 2021-10-14T22:12:25Z

thanks jasha. unfortunately this fix doesn't help when running nox locally. I can actually reproduce this issue running nox on my local machine.

I think this is broken by the latest release pip 21.3, pinning pip==21.2.4 seems to resolve the issue for me when running locally.

this issue on pip seems to be related to what we are seeing, although I'm not sure if this is the same issue (we are seeing the failure when install in non editable mode as well.). I tried the fix mentioned in the issue, but the fix only worked in session installing omegaconf in editable mode. Do you mind taking a closer look there? we can add our datapoint if it's not already mentioned in that issue.

for now i suggest we pin the pip version.

jieru-hu

see previous comment.

layday · 2021-10-18T05:34:04Z

In the most recent commit 134be30 I've modified the .circleci/config.yml file to upgrade pip before the nox session starts.

The globally-installed version of pip won't be used to create sessions in nox.

layday · 2021-10-18T06:15:06Z

The issue here is that the CleanCommand from build_helpers is being called while building omegaconf to install it in the nox session, which wipes the .nox folder clean. You are only experiencing this in 21.3, because:

In-tree builds are now the default. --use-feature=in-tree-build is now
ignored. --use-deprecated=out-of-tree-build may be used temporarily to ease
the transition.

... and is unrelated to the editable issue.

layday · 2021-10-18T06:20:14Z

To clarify, previously, pip would make a copy of your entire project folder in $TMPDIR and build the package there. After 21.3, it runs the build from the project's root.

Jasha10 · 2021-10-18T06:21:02Z

I see. Thanks so much for your help @layday!

Jasha10 · 2021-10-18T07:31:36Z

So the root of the problem is that nox calls pip install to install OmegaConf, and pip install calls setup.py's build command. The setup.py build command in-turn calls setup.py's clean command, and this deletes the .nox folder (which should not be deleted while nox is running).

Here are the solutions I can think of:

Decouple setup.py build from setup.py clean: change build_helpers.BuildCommand so that clean does not get called.
Modify setup.py clean: change build_helpers.CleanCommand so that the .nox folder does not get deleted
Modify setup.py build: change build_helpers.BuildCommand so that a modified version of clean gets called (like the normal clean command, but don't delete the .nox folder)
Pin pip!=21.3.0 in the noxfile and hope that the problem goes away once pip 21.3.1 is released (as pip's PR #10577, created to close issue pip#10573 linked above, does indeed make nox tests pass when I run on my local machine, though I certainly don't understand why)

Any thoughts?

layday · 2021-10-18T08:51:02Z

I'd just remove .nox from clean. Its presence doesn't affect the build process and it doesn't get packaged in the sdist by setuptools. If you want to declutter the repo from files which are not generated by setuptools, a setuptools command probably isn't the place to do it.

Jasha10 · 2021-10-18T18:27:49Z

That rationale makes sense to me. Thanks again @layday!

As for running multiple nox sessions one-after-another: It should not be a problem to skip deleting the .nox folder, as it's presence does not affect future nox runs -- running a nox session causes the old venv with the same session name to be deleted before the new venv for that session is created.

layday · 2021-10-18T20:22:46Z

It looks like you're not using setup.py test, so you could remove setup_requires and test_requires and avoid having to pin pip for editable installs. pytest-runner is deprecated, see [1].

Jasha10 · 2021-10-18T21:53:54Z

Yes indeed, it seems that not only is pytest-runner deprecated, but also:

setup.py test is deprecated
The setuptools.setup keyword argument setup_requires is deprecated.
The setuptools.setup keyword argument tests_require is deprecated.

@omry, do you know if our downstream dependents are relying on omegaconf's setup.py test command? If not, we can remove setup_requires and tests_require from the setup.py file so that pip will not need to be pinned for editable installs in the noxfile.

Jasha10

Notes for reviewers of this PR:

Jasha10 · 2021-10-18T21:58:32Z

.flake8

-exclude = .git,.nox,.tox,omegaconf/grammar/gen
+exclude = .git,.nox,.tox,omegaconf/grammar/gen,build


As of pip version 21.3, in-tree builds are the default for pip, so a build folder may be created.
Linters should ignore this folder.

Jasha10 · 2021-10-18T22:04:12Z

noxfile.py

+    if editable_install:
+        # Editable installs not working in pip 21.3.0, https://github.com/pypa/pip/issues/10573
+        session.install("--upgrade", "setuptools", "pip!=21.3.0")


Pinning pip version here because:
pip 21.3.0 crashes when an editable install is performed on packages that both have a pyproject.toml file and have a setup.py file that uses the deprecated setup_requires keyword argument when calling the setuptools.setup function.
This will probably be fixed in pip version 21.3.1.

As suggested in the discussion on this PR, an alternative to pinning pip here would be to remove the setup_requires=["pytest-runner"] keyword argument from our setup.py file. This means that the (deprecated) setup.py test command would no longer work.

omry

@omry, do you know if our downstream dependents are relying on omegaconf's setup.py test command? If not, we can remove setup_requires and tests_require from the setup.py file so that pip will not need to be pinned for editable installs in the noxfile.

I don't know. Maybe the conda package.

omry · 2021-10-18T22:09:12Z

pyproject.toml

    | \.mypy_cache
-    | \omegaconf/grammar/gen
+    | omegaconf/grammar/gen
    | \.nox


Why this change?

It's unrelated -- just fixing a typo.
There should be no backslash. The other lines have a backslash to escape the period.

omry · 2021-10-18T22:10:08Z

build_helpers/build_helpers.py

@@ -79,7 +79,6 @@ def run(self) -> None:
                "^omegaconf\\.egg-info$",
                "\\.eggs$",
                "^\\.mypy_cache$",
-                "^\\.nox$",


Why this change?

Previously, when running nox with pip version 21.3.0:

When the noxfile installs omegaconf, setup.py build gets called.

When build_helpers:BuildCommand gets called, it invokes build_helpers.CleanCommand.

build_helpers.CleanCommand deletes the .nox folder

This makes nox crash.

I think this was not an issue before because older versions of pip did not do in-tree builds, i.e. the source tree would get copied to a temporary directory before the build/clean command were invoked.

Jasha10 · 2021-10-19T08:39:05Z

I don't know. Maybe the conda package.

Looking at the recipe in the omegaconf-feedstock repo, I'm confident that setup.py test is not being used there.

empty commit to trigger CI

d2fae26

noxfile: dont install pip

951253e

Jasha10 marked this pull request as ready for review October 14, 2021 17:45

Jasha10 requested a review from jieru-hu October 14, 2021 17:45

Jasha10 marked this pull request as draft October 14, 2021 18:24

Jasha10 added 2 commits October 14, 2021 13:45

copy hydra.git/noxfile.py: use session.run instead of session.install

9adc80d

upgrade pip before installing nox

134be30

Jasha10 marked this pull request as ready for review October 14, 2021 19:25

jieru-hu requested changes Oct 14, 2021

View reviewed changes

This was referenced Oct 18, 2021

Interpolation grammar: allow pipe | in unquoted strings #799

Merged

21.3: Breaking editable installation with No module named pip error pypa/pip#10573

Closed

Jasha10 requested a review from pixelb October 18, 2021 07:32

setuptools: don't delete .nox direcetory

a218039

Jasha10 requested a review from jieru-hu October 18, 2021 18:28

Jasha10 added 2 commits October 18, 2021 13:43

pin pip if editable_install

1b4c046

linters ignore build folder

2a32324

Jasha10 commented Oct 18, 2021

View reviewed changes

omry reviewed Oct 18, 2021

View reviewed changes

remove setup.py test and unpin pip

5617067

Jasha10 merged commit fdb78fa into omry:master Oct 19, 2021

Jasha10 deleted the CI_failures branch October 19, 2021 20:15

Jasha10 mentioned this pull request Oct 19, 2021

Fix CI on 2.1 branch #809

Merged

Jasha10 added a commit to Jasha10/omegaconf that referenced this pull request Oct 19, 2021

changes from omry#800

0768915

Jasha10 mentioned this pull request Oct 19, 2021

clarify message re OC_CAUSE=1 env var #806

Merged

Jasha10 changed the title ~~empty commit to trigger CI~~ Fix CI Oct 20, 2021

This was referenced Oct 20, 2021

Remove dependency on pytest-runner conda-forge/omegaconf-feedstock#23

Closed

Fix CI facebookresearch/hydra#1857

Merged

This was referenced Nov 2, 2021

Add documentation for the OmegaConf grammar #792

Merged

Improve Optional[type] support #749

Merged

This was referenced Dec 5, 2021

2.1_branch CI: typo in black config regex #836

Closed

2.1_branch CI: typo in black config regex #837

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix CI #800

Fix CI #800

Jasha10 commented Oct 12, 2021

Jasha10 commented Oct 12, 2021 •

edited

Loading

omry commented Oct 13, 2021

jieru-hu commented Oct 14, 2021

Jasha10 commented Oct 14, 2021

jieru-hu commented Oct 14, 2021

Jasha10 commented Oct 14, 2021

Jasha10 commented Oct 14, 2021

jieru-hu commented Oct 14, 2021

Jasha10 commented Oct 14, 2021

jieru-hu commented Oct 14, 2021

jieru-hu left a comment

layday commented Oct 18, 2021

layday commented Oct 18, 2021

layday commented Oct 18, 2021

Jasha10 commented Oct 18, 2021

Jasha10 commented Oct 18, 2021 •

edited

Loading

layday commented Oct 18, 2021

Jasha10 commented Oct 18, 2021

layday commented Oct 18, 2021

Jasha10 commented Oct 18, 2021

Jasha10 left a comment •

edited

Loading

Jasha10 Oct 18, 2021

Jasha10 Oct 18, 2021 •

edited

Loading

omry left a comment

omry Oct 18, 2021

Jasha10 Oct 18, 2021

omry Oct 18, 2021

Jasha10 Oct 18, 2021 •

edited

Loading

Jasha10 commented Oct 19, 2021

		exclude = .git,.nox,.tox,omegaconf/grammar/gen
		exclude = .git,.nox,.tox,omegaconf/grammar/gen,build

Fix CI #800

Fix CI #800

Conversation

Jasha10 commented Oct 12, 2021

Jasha10 commented Oct 12, 2021 • edited Loading

omry commented Oct 13, 2021

jieru-hu commented Oct 14, 2021

Jasha10 commented Oct 14, 2021

jieru-hu commented Oct 14, 2021

Jasha10 commented Oct 14, 2021

Jasha10 commented Oct 14, 2021

jieru-hu commented Oct 14, 2021

Jasha10 commented Oct 14, 2021

jieru-hu commented Oct 14, 2021

jieru-hu left a comment

Choose a reason for hiding this comment

layday commented Oct 18, 2021

layday commented Oct 18, 2021

layday commented Oct 18, 2021

Jasha10 commented Oct 18, 2021

Jasha10 commented Oct 18, 2021 • edited Loading

layday commented Oct 18, 2021

Jasha10 commented Oct 18, 2021

layday commented Oct 18, 2021

Jasha10 commented Oct 18, 2021

Jasha10 left a comment • edited Loading

Choose a reason for hiding this comment

Jasha10 Oct 18, 2021

Choose a reason for hiding this comment

Jasha10 Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

omry left a comment

Choose a reason for hiding this comment

omry Oct 18, 2021

Choose a reason for hiding this comment

Jasha10 Oct 18, 2021

Choose a reason for hiding this comment

omry Oct 18, 2021

Choose a reason for hiding this comment

Jasha10 Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

Jasha10 commented Oct 19, 2021

Jasha10 commented Oct 12, 2021 •

edited

Loading

Jasha10 commented Oct 18, 2021 •

edited

Loading

Jasha10 left a comment •

edited

Loading

Jasha10 Oct 18, 2021 •

edited

Loading

Jasha10 Oct 18, 2021 •

edited

Loading