[dg] Fail on dagster-dg version less than dagster version (BUILD-1041) #29505
+85
−11
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Apr 22, 2025
Collaborator
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
smackesey
changed the title
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
smackesey
force-pushed
the
sean/dg/warn-on-dg-less-than-dagster
branch
3 times, most recently
from
b1eaa15 to
f005d06
Compare
smackesey
force-pushed
the
sean/dg/warn-on-outdated-dg
branch
from
7115b58 to
bc9aa28
Compare
smackesey
force-pushed
the
sean/dg/warn-on-dg-less-than-dagster
branch
from
f005d06 to
4022708
Compare
smackesey
force-pushed
the
sean/dg/warn-on-outdated-dg
branch
2 times, most recently
from
10db195 to
54187a2
Compare
smackesey
force-pushed
the
sean/dg/warn-on-dg-less-than-dagster
branch
from
4022708 to
6d831b4
Compare
smackesey
force-pushed
the
sean/dg/warn-on-outdated-dg
branch
from
54187a2 to
c4e63c4
Compare
smackesey
force-pushed
the
sean/dg/warn-on-dg-less-than-dagster
branch
from
6d831b4 to
f743661
Compare
smackesey
force-pushed
the
sean/dg/warn-on-outdated-dg
branch
from
c4e63c4 to
8c10dc6
Compare
smackesey
force-pushed
the
sean/dg/warn-on-dg-less-than-dagster
branch
from
424530f to
3ab6f73
Compare
gibsondan
requested changes
Apr 23, 2025
Member
gibsondan
left a comment
gibsondan
left a comment
There was a problem hiding this comment.
Implementation seems fine but i'm worried the error isn't clear enough about what's happening
| if dagster_dg_version < minimum_dagster_dg_version: | ||
| exit_with_error( | ||
| f""" | ||
| dagster-dg version is incompatible with the installed version of dagster. |
Member
There was a problem hiding this comment.
I think the copy here could be wordsmithed a bit. This reads to me like a problem with the venv in which dg is running, as opposed to a compatibility problem between two different venvs.
smackesey
force-pushed
the
sean/dg/warn-on-dg-less-than-dagster
branch
from
3ab6f73 to
b258773
Compare
smackesey
force-pushed
the
sean/dg/warn-on-outdated-dg
branch
from
8c10dc6 to
5bbe1e2
Compare
benpankow
approved these changes
Apr 23, 2025
| if dagster_dg_version < minimum_dagster_dg_version: | ||
| exit_with_error( | ||
| textwrap.dedent(f""" | ||
| Current `dg` version is incompatible with `dagster-components` version in the resolved environment. |
Member
There was a problem hiding this comment.
Can we drop the reference to dagster-components CLI entirely, or at least in the preamble? That feels like an implementation detail and IMO reads at first like it refers to a separate dagster-components package.
Current dg version is incompatible with dagster version in the resolved environment.
dgcommunicates across a process boundary withdagster. Found versions:
(Maybe this is a reason to rename the dagster-components entrypoint entirely)
Collaborator
Author
There was a problem hiding this comment.
Yeah changed it-- we should probably just fold dagster-components onto a dg-server subcommand of dagster or something.
smackesey
force-pushed
the
sean/dg/warn-on-dg-less-than-dagster
branch
from
b258773 to
aebd2fe
Compare
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
smackesey
force-pushed
the
sean/dg/warn-on-dg-less-than-dagster
branch
from
aebd2fe to
2622ef1
Compare
gibsondan
approved these changes
Apr 23, 2025
smackesey
added a commit
that referenced
this pull request
Apr 23, 2025
#29505) ## Summary & Motivation Exit with an error when the `dg` version is less than the dagster in the in-scope environment. We have to transform the "core version" of `dagster` to a "library version" in order to perform the comparison. The check is run whenever we spawn a `dagster-components` subprocess. ## How I Tested These Changes New unit tests. ## Changelog `dg` will now fail with an error message if it's version is below the minimum supported version for the version of `dagster` in your environment. --------- Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary & Motivation
Exit with an error when the
dgversion is less than the dagster in the in-scope environment. We have to transform the "core version" ofdagsterto a "library version" in order to perform the comparison.The check is run whenever we spawn a
dagster-componentssubprocess.How I Tested These Changes
New unit tests.
Changelog
dgwill now fail with an error message if it's version is below the minimum supported version for the version ofdagsterin your environment.