[engsys] Migrate to tox4 #30159

kdestin · 2023-04-30T23:32:44Z

Description

This pull request migrates this repository's tox config to be compatible with tox 4. Major changes include:

All references to tox-monorepo were removed in the repository. tox4 adds native support for the use-case that tox-monorepo enables: --root allows for {toxinidir} to be specified indepedently of the location of tox.ini.
Documentation was updated to use tox4-isms: use --root in examples, call out that default values for the config file and root can be set with environment variables, etc...
tox_harness.py was updated to use --root when invoking tox
A smattering of refactors to tox.ini to clean it up
- As many paths as possible were re-written relative to repository root. (Uses a local tox plugin (toxfile.py) to compute the repository root)
- Deprecated and/or unnecessary config keys were updated.

This PR is part of an ongoing effort by the ML team to make azure-sdk-for-python's tooling more convenient for use during local development (Not a high priority workstream): #28785.

Resolves #28267

cc: @scbedd

All SDK Contribution checklist:

The pull request does not introduce [breaking changes]
CHANGELOG is updated for new features, bug fixes or other significant changes.
I have read the contribution guidelines.

General Guidelines and Best Practices

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

Pull request includes test coverage for the included changes.

tox prepends the {env_bin_dir} to $PATH, so commands will default to using the virtual environment first. See: https://tox.wiki/en/4.4.11/config.html#commands One of tox's core maintainers also discourages its use: tox-dev/tox#2909 (comment)

Uses an inline plugin (toxfile.py) to make a computed config value avaiable to tox environments.

{toxinidir} is an alias for {tox_root}, but {tox_root} is: * More clear: {tox_root} isn't necessarily where the ini file is * Shorter

This is the default behavior, see: https://tox.wiki/en/4.4.11/config.html#change_dir-external

See: https://tox.wiki/en/latest/upgrading.html#removed-tox-ini-keys

--ignore-installed can break a python installation if multiple conflicting versions of a package are installed

platform = linux: linux macos: darwin windows: win32 Setting the above in the base environment has no effect: * None of the environments have {linux,macos,windows} in the name, so the platform config is always empty.

tox4 natively solves what `tox-monorepo` was written to solve: `--root` lets you set `toxinidir` independently of the config file in use. 4.4.11 was chosen as the cutoff since it was the first release to include a fix to `--root` that prevented `{work_dir}` from being changed when `{toxinidir}` was changed by `--root`. `tox-monorepo` should no longer be needed

* References to tox-monorepo were removed * Replaced discussion of `tox -l` with tox4's `tox list` * Updated examples to use `--root`

…point to CONTRIBUTING.md for tox

* TOX_CONFIG_DIR can be used to permanently set --conf * TOX_ROOT_DIR can be used to permanently set --root

scbedd · 2023-05-10T20:16:48Z

Another thing that's visible is that we are seeing timeouts again when installing.
There has to be something that isn't shared properly in the new tox, which is why we see these really super long timeouts in CI. It's a deadlock between two tox environments running in parallel :(

Do you happen to know what the issue was the last time you dealt with this?

It was a long painful process of nailing down every single possible file lock. That's why you notice stuff like a different pip cache directory for each tox environment, etc.

sdk/core/azure-core/tests/async_tests/test_rest_response_backcompat_async.py

eng/regression_test_tools.txt

eng/regression_tools.txt

scbedd · 2023-05-11T18:09:45Z

We're v close now! Just FYI we won't be merging this even if it's green until next week at the least. We're in a release window right now and I don't like to introduce any eng code changes around this time.

Remaining issues and additional checks before we can merge this:

Resolve autorest - pr failing on this PR
- ~~I think we will be able to resolve this by a simple update from upstream once we iron out everything else.~~
- PR Already Submit By @iscai-msft
  - Gonna get this merged to resolve that failure.
Check that our regression tests will still run. Entrypoint. Implementation.
Run nightly alpha version (storage,core,ml) to confirm everything works there. (internal build, SetDevVersion=true, run)
- Storage
- Core
- ML
- These are all running with Run.Regression=true and SetDevVersion=true
Resolve failing coverage
- ~~I'm looking at this right now, it's probably a fix where we don't need to apply our fix .coverage files anymore~~
Why is videoanalyzer CI timing out so badly? Fix it!
- I believe I added a commit that resolves this. azure-media-videoanalyzer-edge dev_requirements included tox>4.4.X, which was causing us to attempt to install tox within a tox venv. Pretty clear that's where the massive timeout was coming from.

…ed namespace (Azure#30344)

eng/tox/tox.ini

kdestin · 2023-05-12T00:32:58Z

We're v close now!

Nice!

Just FYI we won't be merging this even if it's green until next week at the least. We're in a release window right now and I don't like to introduce any eng code changes around this time.

Works for me, there's no rush on my end.

Why is videoanalyzer CI timing out so badly? Fix it!

I believe I added a commit that resolves this. azure-media-videoanalyzer-edge dev_requirements included tox>4.4.X, which was causing us to attempt to install tox within a tox venv. Pretty clear that's where the massive timeout was coming from.

Wow, I would've imagined that it trivially satisfies that installation requirement (I think tox bootstraps itself into a virtualenv, but I may be wrong).

scbedd · 2023-05-12T01:05:14Z

Wow, I would've imagined that it trivially satisfies that installation requirement (I think tox bootstraps itself into a virtualenv, but I may be wrong).

At the end of the day it's just using pip to install within the environment, so there is no place to prevent that install loop from going wrong. I'm almost certain this is related to a file lock someplace. Regardless, cutting that entry resolves the issue. I think we're probably ok on the parallel access problem I thought may be present.

scbedd · 2023-05-12T01:08:55Z

I will run regression and nightly alpha checks tomorrow, then we will be in the clear to merge this next week! 🚀 🚀 🚀 🚀 🚀 🚀 🚀 🚀

scbedd · 2023-05-12T21:33:41Z

@kdestin We are GREEN! Regress tests + nightly alpha versions all seem to be working appropriately!

I'm a bit disturbed by the consistent credscan stack overflow, but I don't think that's related to this PR at all. Let's see where releases are come monday, then make an announcement and merge it!

Thank you so much for all the effort on this @kdestin ! This is a gigantic necessary contribution that is not at all fun to slog through 👍

scbedd · 2023-05-15T20:12:32Z

@kdestin We are GREEN! Regress tests + nightly alpha versions all seem to be working appropriately!

I'm a bit disturbed by the consistent credscan stack overflow, but I don't think that's related to this PR at all. Let's see where releases are come monday, then make an announcement and merge it!

Thank you so much for all the effort on this @kdestin ! This is a gigantic necessary contribution that is not at all fun to slog through 👍

On detailed review over the weekend, I found one last issue.

We have one outstanding issue with coverage not properly generating. I think it's simply because we need to install the packages that we generate coverage for, but I'm going to figure that out.

Remember, package is installed when we GENERATE the coverage, but I think we now need to ensure that its present when we COMBINE and PUBLISH this report. I think that's the issue at play.

… combining .coverage files. amend our tox tree cleanup to exclude the whl directory when running coverage.

scbedd · 2023-05-18T17:20:55Z

Ok @kdestin I think we are ready to rock!

Barring a notification to the internal team, this is ready to squash merge!

scbedd · 2023-05-22T23:42:40Z

/azp run python - core - ci

azure-pipelines · 2023-05-22T23:42:59Z

Azure Pipelines successfully started running 1 pipeline(s).

lmazuel

Thanks again @kdestin !

kdestin added 19 commits April 30, 2023 19:08

Remove references to {envbindir}

1910a58

tox prepends the {env_bin_dir} to $PATH, so commands will default to using the virtual environment first. See: https://tox.wiki/en/4.4.11/config.html#commands One of tox's core maintainers also discourages its use: tox-dev/tox#2909 (comment)

refactor: Write paths relative to git root, instead of package dir

3a4449d

Uses an inline plugin (toxfile.py) to make a computed config value avaiable to tox environments.

refactor: Rename {toxinidir} to {tox_root}

e4d6043

{toxinidir} is an alias for {tox_root}, but {tox_root} is: * More clear: {tox_root} isn't necessarily where the ini file is * Shorter

refactor: Flatten some commands that are no longer super long

c453c0b

refactor: Make a pytest section

04a8b78

refactor: Remove changedir = {tox_root}

7d3da58

This is the default behavior, see: https://tox.wiki/en/4.4.11/config.html#change_dir-external

fix: {distdir} is deprecated

48291cd

See: https://tox.wiki/en/latest/upgrading.html#removed-tox-ini-keys

fix: Remove --ignore-installed from pip command

c0fd18b

--ignore-installed can break a python installation if multiple conflicting versions of a package are installed

refactor: Add descriptions to all tox environments

df6b428

fix: Remove ineffective platform config

0280e05

platform = linux: linux macos: darwin windows: win32 Setting the above in the base environment has no effect: * None of the environments have {linux,macos,windows} in the name, so the platform config is always empty.

docs: Update CONTRIBUTING.md to use tox 4 commands

796e553

* References to tox-monorepo were removed * Replaced discussion of `tox -l` with tox4's `tox list` * Updated examples to use `--root`

docs: Make azure-media-{videoanalyzer,analytics}-edge's DevTips.md …

d4966e7

…point to CONTRIBUTING.md for tox

docs: Remove remaining references to tox-monorepo

47e62a0

doc: Use --root throughout documentation

ff88741

engsys: Make tox_harness set {toxinidir} when invoking tox

4f99e5a

engsys: Set required tox version in config

236e179

docs: Use the new url for tox docs

c27d324

docs: Add a callout that TOX_* environment variables control defaults

e9929cb

* TOX_CONFIG_DIR can be used to permanently set --conf * TOX_ROOT_DIR can be used to permanently set --root

kdestin requested review from lmazuel, mccoyp, scbedd, weshaggard, benbp, hivyas and giakas as code owners April 30, 2023 23:32

github-actions bot added VideoAnalyzer Azure Video Analyzer Media labels Apr 30, 2023

kdestin removed VideoAnalyzer Azure Video Analyzer Media labels Apr 30, 2023

scbedd reviewed May 10, 2023

View reviewed changes

sdk/core/azure-core/tests/async_tests/test_rest_response_backcompat_async.py Show resolved Hide resolved

pin coverage to newer version with no conflicts

8edb70b

kdestin commented May 10, 2023

View reviewed changes

eng/regression_test_tools.txt Outdated Show resolved Hide resolved

eng/regression_tools.txt Outdated Show resolved Hide resolved

scbedd added 4 commits May 11, 2023 11:48

identify issue with missing code coverage being caused by wrongly nam…

32cdba9

…ed namespace (Azure#30344)

remove fix of dotfile!

f298007

apply black formatting to resolve failing analyze step

a6ad545

tox installing tox. probably not a good thing :)

2c2dd28

kdestin commented May 12, 2023

View reviewed changes

eng/tox/tox.ini Show resolved Hide resolved

kdestin and others added 2 commits May 12, 2023 12:56

engsys: Remove tox from azure-media-analytics-edge dev_requirements

c1ffbc9

align regression version of ci/test_tools.txt

5d6243e

scbedd added 4 commits May 16, 2023 18:36

new coverage format requires that the package sources be present when…

9f74919

… combining .coverage files. amend our tox tree cleanup to exclude the whl directory when running coverage.

ensure that missing hidden folder can't break cleanup operation

c278ce5

ensure coverage isn't generated on whl_no_aio

c297c9d

ensure that coverage is generated without wonky package paths

d506e99

lmazuel approved these changes May 23, 2023

View reviewed changes

lmazuel merged commit 982234b into Azure:main May 23, 2023
219 checks passed

scbedd mentioned this pull request May 23, 2023

Update pytest related dependencies, coverage especially #27828

Closed

2 tasks

This was referenced May 24, 2023

Update tox-monorepo for tox 4.0, update CI to consume tox 4.0 #28267

Closed

[engsys] fix: Broken next-pylint tox environment #30526

Closed

tchia04 mentioned this pull request Feb 20, 2024

tox-monorepo not compatible with Tox 4 Azure/azure-sdk-tools#4917

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[engsys] Migrate to tox4 #30159

[engsys] Migrate to tox4 #30159

kdestin commented Apr 30, 2023 •

edited

scbedd commented May 10, 2023 •

edited

scbedd commented May 11, 2023 •

edited

kdestin commented May 12, 2023

scbedd commented May 12, 2023

scbedd commented May 12, 2023

scbedd commented May 12, 2023

scbedd commented May 15, 2023

scbedd commented May 18, 2023

scbedd commented May 22, 2023

azure-pipelines bot commented May 22, 2023

lmazuel left a comment

[engsys] Migrate to tox4 #30159

[engsys] Migrate to tox4 #30159

Conversation

kdestin commented Apr 30, 2023 • edited

Description

All SDK Contribution checklist:

General Guidelines and Best Practices

Testing Guidelines

scbedd commented May 10, 2023 • edited

scbedd commented May 11, 2023 • edited

kdestin commented May 12, 2023

scbedd commented May 12, 2023

scbedd commented May 12, 2023

scbedd commented May 12, 2023

scbedd commented May 15, 2023

scbedd commented May 18, 2023

scbedd commented May 22, 2023

azure-pipelines bot commented May 22, 2023

lmazuel left a comment

Choose a reason for hiding this comment

kdestin commented Apr 30, 2023 •

edited

scbedd commented May 10, 2023 •

edited

scbedd commented May 11, 2023 •

edited