Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to tox 4 #23449

Merged
merged 3 commits into from
Aug 7, 2024
Merged

Upgrade to tox 4 #23449

merged 3 commits into from
Aug 7, 2024

Conversation

PedramNavid
Copy link
Contributor

Summary & Motivation

Tox 3 was last updated in 2022. It's time to move to tox 4. The only breaking change is passenv variables need to be on a new line

How I Tested These Changes

bk

@graphite-app graphite-app bot added the area: docs Related to documentation in general label Aug 6, 2024
@graphite-app graphite-app bot requested a review from erinkcochran87 August 6, 2024 19:18
@PedramNavid PedramNavid removed the request for review from erinkcochran87 August 6, 2024 19:18
Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @PedramNavid and the rest of your teammates on Graphite Graphite

Copy link

github-actions bot commented Aug 6, 2024

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-65h8u3o0v-elementl.vercel.app
https://pdrm-tox-4.components-storybook.dagster-docs.io

Built with commit 7275325.
This pull request is being automatically deployed with vercel-action

Copy link
Contributor

@rexledesma rexledesma left a comment

Choose a reason for hiding this comment

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

We should make this run against the full suite + all python versions just to double check that everything is fine.

You can add a magic string in your commit message to do that: https://www.notion.so/dagster/Buildkite-01a5666c2527462db2c3abe0d98cde45?pvs=4#55a86580485948c2877c55e62fe7fa39

Copy link

github-actions bot commented Aug 6, 2024

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-gu13jyx8o-elementl.vercel.app
https://pdrm-tox-4.dagster.dagster-docs.io

Direct link to changed pages:

@PedramNavid
Copy link
Contributor Author

We should make this run against the full suite + all python versions just to double check that everything is fine.

You can add a magic string in your commit message to do that: https://www.notion.so/dagster/Buildkite-01a5666c2527462db2c3abe0d98cde45?pvs=4#55a86580485948c2877c55e62fe7fa39

good call, just ran

@PedramNavid PedramNavid requested a review from rexledesma August 6, 2024 22:40
Copy link
Contributor

@rexledesma rexledesma left a comment

Choose a reason for hiding this comment

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

Sweet, thanks! Is there a corresponding PR for internal as well? Going to request changes just to make sure that's present.

  • Let's ensure tox>=4 just to be safe
  • Let's update tox.ini.tmpl in the python package scaffold as well

python_modules/dagster/setup.py Outdated Show resolved Hide resolved
@PedramNavid
Copy link
Contributor Author

PedramNavid commented Aug 6, 2024

Sweet, thanks! Is there a corresponding PR for internal as well? Going to request changes just to make sure that's present.

  • Let's ensure tox>=4 just to be safe
  • Let's update tox.ini.tmpl in the python package scaffold as well

Added! TIL about the python package scaffold. This whole time I was copy pasting files from other packages that looked decent 😭

Copy link
Contributor

@rexledesma rexledesma left a comment

Choose a reason for hiding this comment

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

yahoo

@schrockn
Copy link
Member

schrockn commented Aug 7, 2024

What are the concrete benefits we get out of the upgrade?

Copy link
Contributor Author

What are the concrete benefits we get out of the upgrade?

There’s some performance improvements to start times and better handling of dependency changes, eg when adding a new package while developing.

But really, I sometimes end up with tox 4 in my environment somehow and it would never run and I’d end up reinstalling 3.2.5 which annoyed me, which was the real motivation for this

@schrockn
Copy link
Member

schrockn commented Aug 7, 2024

Makes sense! Obviously in support. Just wondering what concrete goodies we get.

Copy link
Contributor Author

Makes sense! Obviously in support. Just wondering what concrete goodies we get.

Ohh there's also this:
https://github.com/tox-dev/tox-uv
which requires tox>=4.16

Copy link
Contributor Author

PedramNavid commented Aug 7, 2024

Merge activity

@PedramNavid PedramNavid merged commit 06d5afe into master Aug 7, 2024
2 of 3 checks passed
@PedramNavid PedramNavid deleted the pdrm/tox-4 branch August 7, 2024 16:17
@rexledesma
Copy link
Contributor

FYI we already use uv in tox: #19871

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: docs Related to documentation in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants