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.

Sign up for GitHub

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

Jump to bottom

[components] Add windows testing to dagster-dg (BUILD-672) #27537

Open
wants to merge 1 commit into
base: graphite-base/27537
Choose a base branch
from
Open

[components] Add windows testing to dagster-dg (BUILD-672) #27537

wants to merge 1 commit into from
+385 −167

Conversation

smackesey
Copy link
Collaborator

@smackesey smackesey commented Feb 4, 2025 •
edited
Loading

Summary & Motivation

dagster-dg is currently broken on Windows. I found this out by trying to add Windows testing. This PR fixes dg for Windows and adds Windows testing.

  • Modify our azure pipeline specification to also test dagster-dg.
  • Make sure all paths are handled with Path and don't contain embedded "/"
  • Handle process shutdown appropriately on Windows for dg dev
  • Some adjustments to the tests of console output, since sometimes the box character used to draw tables and panels differ on Windows.

How I Tested These Changes

Test suite now passes on Windows.

This was referenced Feb 4, 2025
Merged
Merged
Merged
Draft
@smackesey Graphite App
Copy link
Collaborator Author

smackesey commented Feb 4, 2025 •
edited
Loading

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

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

@smackesey smackesey marked this pull request as ready for review February 4, 2025 01:52
@smackesey smackesey force-pushed the windows/components/add-windows-testing branch from c5f92c5 to 9894ca5 Compare February 4, 2025 02:00
@smackesey smackesey changed the title [components] Add windows testing to dagster-dg [components] Add windows testing to dagster-dg (BUILD-672) Feb 4, 2025
@smackesey smackesey force-pushed the sean/components/dg-dev branch from 0ad8ae0 to 81af794 Compare February 4, 2025 02:29
@smackesey smackesey force-pushed the windows/components/add-windows-testing branch 4 times, most recently from 7dbac06 to 9995d6c Compare February 4, 2025 16:09
@smackesey smackesey force-pushed the sean/components/dg-dev branch from 81af794 to 594bc15 Compare February 4, 2025 16:09
graphite-app[bot]
graphite-app bot reviewed Feb 4, 2025
View reviewed changes
python_modules/libraries/dagster-dg/dagster_dg_tests/utils.py Outdated

venv_bin = Path.cwd() / ".venv" / "bin"
with modify_environment_variable(
"PATH", os.pathsep.join([str(venv_bin), os.pathsep, os.environ["PATH"]])
Copy link

Choose a reason for hiding this comment

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

The PATH construction has an extra separator due to os.pathsep being included in the join list. This creates paths like /venv/bin::/usr/bin with double separators. The correct construction should be:

os.pathsep.join([str(venv_bin), os.environ['PATH']])

Spotted by Graphite Reviewer

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

@smackesey smackesey force-pushed the windows/components/add-windows-testing branch from 9995d6c to f13115a Compare February 4, 2025 16:13
Base automatically changed from sean/components/dg-dev to master February 4, 2025 16:49
@smackesey smackesey force-pushed the windows/components/add-windows-testing branch 12 times, most recently from f4597f5 to a17e230 Compare February 7, 2025 03:26
graphite-app[bot]
graphite-app bot reviewed Feb 7, 2025
View reviewed changes
python_modules/libraries/dagster-dg/dagster_dg_tests/cli_tests/test_custom_help_format.py Outdated
Comment on lines 125 to 121
def test_join_path():
from pathlib import Path

my_path = Path("foo/bar")
updated_path = my_path / "baz"
print(updated_path)
assert False

Copy link

Choose a reason for hiding this comment

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

This test appears to be a debugging function that was inadvertently committed. It contains a forced failure (assert False) and debug print statements, suggesting it was used to investigate Path joining behavior. Consider removing this test or converting it into meaningful test coverage if path joining behavior needs verification.

Spotted by Graphite Reviewer

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

@smackesey smackesey force-pushed the windows/components/add-windows-testing branch from 2ea7cd6 to d56aef2 Compare February 9, 2025 19:22
graphite-app[bot]
graphite-app bot reviewed Feb 9, 2025
View reviewed changes
python_modules/libraries/dagster-dg/dagster_dg/cache.py Outdated
Comment on lines 98 to 99
def _get_path(self, key: tuple[str, ...]) -> Path:
my_path = self._root_path
for k in key:
my_path = my_path / k
return Path(self._root_path, *key)
Copy link

Choose a reason for hiding this comment

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

The code builds a path in my_path but then returns a new Path object constructed from self._root_path and key. This appears to be unintentional - the return value should be my_path since it represents the same path being constructed.

Spotted by Graphite Reviewer

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

@smackesey smackesey force-pushed the graphite-base/27537 branch from 6d6358f to c72b34d Compare February 9, 2025 20:34
@smackesey smackesey force-pushed the windows/components/add-windows-testing branch 2 times, most recently from a14eedf to f994711 Compare February 9, 2025 21:28
@smackesey smackesey force-pushed the graphite-base/27537 branch 2 times, most recently from 5f7a006 to 0ffd2a2 Compare February 9, 2025 21:55
@smackesey smackesey force-pushed the windows/components/add-windows-testing branch 2 times, most recently from 9679ca7 to 0aff97e Compare February 9, 2025 22:24
@smackesey smackesey force-pushed the graphite-base/27537 branch from 0ffd2a2 to b3b82c4 Compare February 9, 2025 22:24
@smackesey smackesey force-pushed the windows/components/add-windows-testing branch from 0aff97e to 0994542 Compare February 9, 2025 22:49
@smackesey smackesey force-pushed the graphite-base/27537 branch from b3b82c4 to d2046a1 Compare February 9, 2025 22:49
@smackesey smackesey force-pushed the windows/components/add-windows-testing branch from 0994542 to 608c006 Compare February 10, 2025 03:24
@smackesey smackesey force-pushed the windows/components/add-windows-testing branch from 608c006 to cdf9c8d Compare February 10, 2025 03:35
@smackesey smackesey force-pushed the graphite-base/27537 branch 2 times, most recently from 8841273 to e2d78a6 Compare February 10, 2025 04:24
@smackesey smackesey force-pushed the windows/components/add-windows-testing branch from cdf9c8d to 25c7c84 Compare February 10, 2025 04:24
schrockn
schrockn reviewed Feb 10, 2025
View reviewed changes
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.

Wow thanks for powering through this.

This PR is definitely spooky in terms of the amount of complextiy we are going to have to manage here. Does this also give us any pause as to the multi-process nature of dg dev?

Adding @gibsondan so that he has eyes on this.

@schrockn schrockn requested a review from gibsondan February 10, 2025 11:14
@smackesey
Copy link
Collaborator Author

Does this also give us any pause as to the multi-process nature of dg dev?

I don't think so-- that's essential to reap the benefits of different Python environments per code location.

There's a lot in this PR but the added complexity is mostly at the test level.

@smackesey smackesey force-pushed the windows/components/add-windows-testing branch from 25c7c84 to fb67fa3 Compare February 10, 2025 13:37
@smackesey smackesey mentioned this pull request Feb 10, 2025
Open
@smackesey smackesey force-pushed the windows/components/add-windows-testing branch from fb67fa3 to bd8f823 Compare February 10, 2025 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schrockn schrockn schrockn left review comments

@graphite-app graphite-app[bot] graphite-app[bot] left review comments

@benpankow benpankow Awaiting requested review from benpankow

@OwenKephart OwenKephart Awaiting requested review from OwenKephart

@gibsondan gibsondan Awaiting requested review from gibsondan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@smackesey @schrockn