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

Add --use-editable-components-only flag #28140

Merged
merged 1 commit into from
Mar 2, 2025

Conversation

smackesey
Copy link
Collaborator

@smackesey smackesey commented Feb 28, 2025

Summary & Motivation

Adds a --use-editable-components-only flag to complement --use-editable-dagster.

  • --use-editable-components-only is an option to commands that scaffold projects dg init and dg project scaffold. It will bring in dagster_components as an editable install but not dagster and other libs. This simulates the environment into which dg/components is released, so long as it is on a separate release schedule from Dagster core.
  • Update all tests that were using --use-editable-dagster to instead use --use-editable-components-only.
  • Refactor these options into a shared group.

How I Tested These Changes

New unit tests.

Copy link
Collaborator Author

smackesey commented Feb 28, 2025

@smackesey smackesey force-pushed the sean/components/use-editable-components-only branch 2 times, most recently from aa7232d to 827338c Compare February 28, 2025 22:11
@smackesey smackesey force-pushed the sean/components/reenable-validate-workspace-test branch from 30ea1c2 to 7c2a88f Compare February 28, 2025 22:11
@smackesey smackesey force-pushed the sean/components/use-editable-components-only branch from 827338c to 8c39154 Compare February 28, 2025 22:26
@smackesey smackesey force-pushed the sean/components/reenable-validate-workspace-test branch from 7c2a88f to 9b2617a Compare February 28, 2025 22:26
@smackesey smackesey marked this pull request as ready for review February 28, 2025 22:26
["--disable-cache"],
is_flag=True,
default=DgCliConfig.disable_cache,
help="Disable the cache..",
Copy link
Contributor

Choose a reason for hiding this comment

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

The help text has a double period (..) which appears to be a typo. Should be Disable the cache.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

["--disable-cache"],
is_flag=True,
default=DgCliConfig.disable_cache,
help="Disable the cache..",
Copy link
Contributor

Choose a reason for hiding this comment

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

The help text has a double period (..) which appears to be a typo. Should be "Disable the cache."

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

@smackesey smackesey changed the base branch from sean/components/reenable-validate-workspace-test to graphite-base/28140 February 28, 2025 22:31
@smackesey smackesey force-pushed the sean/components/use-editable-components-only branch from 8c39154 to 071ea34 Compare February 28, 2025 22:31
@smackesey smackesey changed the base branch from graphite-base/28140 to sean/components/reenable-validate-workspace-test February 28, 2025 22:31
@smackesey smackesey force-pushed the sean/components/reenable-validate-workspace-test branch from 5451c5c to 6207228 Compare February 28, 2025 22:48
@smackesey smackesey force-pushed the sean/components/use-editable-components-only branch 2 times, most recently from c157fef to 2a66b9c Compare February 28, 2025 23:00
@schrockn schrockn requested a review from gibsondan March 1, 2025 01:00
# the dependencies themselves differently depending on whether we are using editable dagster or
# not. This is because `tool.uv.sources` only seems to apply to direct dependencies of the package,
# so any 2+-order Dagster dependency of our package needs to be listed as a direct dependency in the
# editable case.
Copy link
Member

Choose a reason for hiding this comment

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

Link to the relevant github issue in the uv repo in the comment

Copy link
Member

Choose a reason for hiding this comment

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

astral-sh/uv#9446 because i had it handy

Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

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

Makes sense. I'd like @gibsondan to take a pass at this before commit as he should be aware of all these venv shenanigans

@smackesey smackesey changed the base branch from sean/components/reenable-validate-workspace-test to graphite-base/28140 March 1, 2025 02:33
@smackesey smackesey force-pushed the graphite-base/28140 branch from 6207228 to b60e419 Compare March 1, 2025 02:34
@smackesey smackesey force-pushed the sean/components/use-editable-components-only branch from 2a66b9c to f676ec0 Compare March 1, 2025 02:34
@smackesey smackesey changed the base branch from graphite-base/28140 to master March 1, 2025 02:34
Copy link
Member

@gibsondan gibsondan left a comment

Choose a reason for hiding this comment

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

ok so it seems like the underlying issue here is that dagster-components is not pinned to dagster like all our other libraries. Is the plan to eventually change that once components dev is more stable?

),
),
click.Option(
["--use-editable-components-only"],
Copy link
Member

Choose a reason for hiding this comment

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

I'm finding the name here pretty confusing in a way that --use-editable-dagster was not, because it makes me think that this is referring to the components themselves, rather than dagster-components the python package.

Maybe this is not the end of the world for a temporary hidden flag that is really only for development, but just flagging. you could consider going even more verbose and make this is like --use-editable-components-package-only

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah I agree it's not clear. Will update to --use-editable-components-package-only.

# the dependencies themselves differently depending on whether we are using editable dagster or
# not. This is because `tool.uv.sources` only seems to apply to direct dependencies of the package,
# so any 2+-order Dagster dependency of our package needs to be listed as a direct dependency in the
# editable case.
Copy link
Member

Choose a reason for hiding this comment

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

astral-sh/uv#9446 because i had it handy

Copy link
Collaborator Author

Yes IIRC the plan is to actually just merge the entire dagster-components package into dagster.

@smackesey smackesey force-pushed the sean/components/use-editable-components-only branch from f676ec0 to fa4fe9a Compare March 2, 2025 17:46
@smackesey smackesey force-pushed the sean/components/use-editable-components-only branch from fa4fe9a to 10ea579 Compare March 2, 2025 17:54
@smackesey smackesey merged commit ff57fe9 into master Mar 2, 2025
6 checks passed
@smackesey smackesey deleted the sean/components/use-editable-components-only branch March 2, 2025 18:09
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