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

editable_mode=compat alternative #19983

Merged
merged 1 commit into from
Feb 23, 2024
Merged

editable_mode=compat alternative #19983

merged 1 commit into from
Feb 23, 2024

Conversation

alangenfeld
Copy link
Member

@alangenfeld alangenfeld commented Feb 22, 2024

Manually muck with the path for our goofy imports instead of relying on --config-settings editable_mode=compat

The motivation for this was to enable uv in tox, but the uv level support is coming soon astral-sh/uv#1460

so we may decide to dismiss this and just stick with compat mode installs

How I Tested These Changes

bk

This was referenced Feb 22, 2024
@alangenfeld
Copy link
Member Author

alangenfeld commented Feb 22, 2024

@alangenfeld alangenfeld mentioned this pull request Feb 22, 2024
@alangenfeld alangenfeld changed the title compat-settings alternative editable_mode=compat alternative Feb 22, 2024
Base automatically changed from al/02-21-_docs_remove_run_attribution_python to master February 22, 2024 18:56
@@ -1 +1,4 @@
uncommitted/
uncommitted/
uncommitted/
Copy link
Collaborator

Choose a reason for hiding this comment

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

?

Copy link
Member Author

Choose a reason for hiding this comment

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

some test in dagster ge does this modification it seems

Comment on lines +45 to +50
def ensure_dagster_aws_tests_import() -> None:
dagster_package_root = (Path(dagster_aws_init_py) / ".." / "..").resolve()
assert (
dagster_package_root / "dagster_aws_tests"
).exists(), "Could not find dagster_aws_tests where expected"
sys.path.append(dagster_package_root.as_posix())
Copy link
Member

Choose a reason for hiding this comment

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

if you forget to add this when adding a new _tests import, what's the experience you get today on BK? It would be sad if it works great until it tries to run on 3.12 in the release branch, then gets a confusing error

Copy link
Member Author

Choose a reason for hiding this comment

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

the uv installs for the default 3.11 fail without this stuff so you should hit it on PR runs

Copy link
Collaborator

@smackesey smackesey left a comment

Choose a reason for hiding this comment

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

Is all the deleted docs_snippets stuff a rebase issue?

I leave it to you whether to merge-- uv is doing about a release a day and I know Charlie is prioritizing this so I wouldn't be surprised if we get editable_mode=compat support in the next 24 hours. But maybe we won't-- either way I think if we do land this just to get things running we should revert to editable_mode=compat when uv support lands.

@alangenfeld
Copy link
Member Author

alangenfeld commented Feb 22, 2024

to me the benefit of getting away from needing editable_mode=compat is that its going to be removed

The compat mode is transitional and will be removed in future versions of setuptools, it exists only to help during the migration period

https://setuptools.pypa.io/en/latest/userguide/development_mode.html#legacy-behavior

Copy link

github-actions bot commented Feb 22, 2024

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-2678p2sjv-elementl.vercel.app
https://al-compat-alt.core-storybook.dagster-docs.io

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

@alangenfeld alangenfeld force-pushed the al/compat-alt branch 2 times, most recently from 80d9675 to f9b4032 Compare February 23, 2024 15:27
Copy link

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-2ya4j6af0-elementl.vercel.app
https://al-compat-alt.components-storybook.dagster-docs.io

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

@alangenfeld alangenfeld merged commit 9953057 into master Feb 23, 2024
1 check passed
@alangenfeld alangenfeld deleted the al/compat-alt branch February 23, 2024 23:08
PedramNavid pushed a commit that referenced this pull request Mar 28, 2024
Manually muck with the path for our goofy imports instead of relying on
`--config-settings editable_mode=compat`

The motivation for this was to enable `uv` in `tox`, but the `uv` level
support is coming soon astral-sh/uv#1460

so we may decide to dismiss this and just stick with compat mode
installs

## How I Tested These Changes

bk
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.

3 participants