move dagster dev tests to core #27654

smackesey · 2025-02-07T15:30:58Z

Summary & Motivation

Our only dagster dev tests currently live in the daemon integration test suite, which isn't run on windows. This moves them to dagster, which is tested on windows.

The tests are mostly moved over as they were, with a few adjustments:

Instead of loading the toys repo, we now just load the simple test repo loaded in the rest of the tests. The toys repo adds a bunch of dependencies but loading it specifically doesn't really test anything relevant.
All tests have been augmented to ensure all child processes are shut down (on Unix at least-- see below)
Some refactoring to make the tests simpler to read.

Previously we weren't testing this on Windows at all, and I went in thinking we would end with exactly the same tests passing on windows and unix. Unfortunately, that's not quite true-- with our current set up I could not find a way to reliably and cleanly shut down the dagster dev process tree on Windows in a CI environment. This has to do with the differences in signal handling and process groups in Windows vs unix. I will quote ChatGPT-o1:

On Windows, CTRL_BREAK_EVENT (and similarly CTRL_C_EVENT) only works when the calling process and the target processes share a console. By default, Python’s subprocess.Popen with CREATE_NEW_PROCESS_GROUP spawns the child into its own new process group but does not automatically attach it to the same console as the parent (and in many CI environments, there may be no real console at all). As a result, sending CTRL_BREAK_EVENT to that child process group essentially does nothing—Windows never delivers the interrupt to those child processes, and they keep running.

The lack of console in the CI environment means that CTRL_BREAK_EVENT, which we use to shut down the child processes, does not propagate directly from dev to the webserver/daemon (but it does appear to work when dagster dev is used interactively, due to sharing a console). So in the Windows tests I ended up explicitly killing all child processes. We could maybe make this more robust in the future by having the dev process send a shutdown signal via another form of IPC, but I couldn't see how to do that here.

How I Tested These Changes

Ran moved dagster dev tests on windows and unix.

smackesey · 2025-02-07T15:31:17Z

[components] Add windows testing to dagster-dg (BUILD-672) #27537 : 2 dependent PRs (#27710 , #27799 )
move dagster dev tests to core #27654 👈 (View in Graphite)
master

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

alangenfeld

what does moving these tests do to the runtime for the associated step in BK?

thanks for digging in to this - unfortunate that there is so much divergence for windows but im not sure theres any better way to deal with it

alangenfeld · 2025-02-10T16:01:52Z

python_modules/dagster/dagster/_serdes/ipc.py

@@ -211,6 +211,14 @@ def interrupt_ipc_subprocess(proc: "Popen[Any]") -> None:
        proc.send_signal(signal.SIGINT)


+def interrupt_or_kill_ipc_subprocess(proc: "Popen[Any]", wait_time: int = 10) -> None:


nit: _then_ instead of _or_ maybe

alangenfeld · 2025-02-10T16:02:33Z

python_modules/dagster/tox.ini

@@ -35,7 +40,7 @@ commands =

  api_tests: pytest -c ../../pyproject.toml -vv ./dagster_tests/api_tests  {env:COVERAGE_ARGS} --durations 10 {posargs}
  asset_defs_tests: pytest -c ../../pyproject.toml -vv ./dagster_tests/asset_defs_tests  {env:COVERAGE_ARGS} --durations 10 {posargs}
-  cli_tests: pytest -c ../../pyproject.toml -vv ./dagster_tests/cli_tests  {env:COVERAGE_ARGS} --durations 10 {posargs}
+  cli_tests: pytest -x -c ../../pyproject.toml -vv ./dagster_tests/cli_tests/  {env:COVERAGE_ARGS} --durations 10 {posargs}


clean this up i assume

alangenfeld · 2025-02-10T16:04:49Z

.../dagster_tests/cli_tests/command_tests/dagster_dev_command_tests/test_dagster_dev_command.py

+    # Skip windows here-- it behaves strangely with respect to child processes and cmdline(). But we
+    # are still checking that all child processes shut down later.


the issue is we just cant exclude the tracker processes? whats up with cmdline? I think its worth being more specific here

Actually I forgot to fully clean this up and had some questions for you about it:

I called these "tracker processes" as a kind of guess just to have a name ~~but I'm not totally sure where they're coming from. I thought maybe from the watch threads of the workspace but those are threads and not separate processes.~~ I think they're from the sensor that issues a RunRequest. They look like this and the number of them varies per test run (I've seen 0, 2, 3):

PID [69896] PPID [69883]: ['/Users/smackesey/stm/code/elementl/oss/.venv/bin/python3', '-c', 'from multiprocessing.spawn import spawn_main; spawn_main(tracker_fd=13, pipe_handle=22)', '--multiprocessing-fork']

I will definitely elaborate the error message. There are two issues on windows:

The transient nature of the "tracker processes" leads to situations where they exist at the time the child processes are gathered but are dead when we check the cmdline(). Checking cmdline() of a dead process throws an error on Windows (only).

For some reason there are ~2x the processes on Windows when running through tox. I think each process we're launching directly goes through a tox shim, which is it's own persistent process that invokes the "real" target process.

When the multiprocessing module is used extra processes are spawned to track certain cross process data structures, so i think you named it those well. No idea whats up with the tox shim thing.

Having to skip out on some of this validation on windows is unfortunate but getting the functionality under tests is a big improvement I am not sure its worth the time it would take to try to improve past what you've cooked up here.

Best thing to do is just leave as much context as you can for the next poor soul who finds them self in this abyss.

smackesey · 2025-02-10T16:42:07Z

what does moving these tests do to the runtime for the associated step in BK?

Increases by a few minutes, but it's still not the slowest dagster BK step

unfortunate that there is so much divergence for windows but im not sure theres any better way to deal with it

One way would be to use dedicated IPC (pipe or socket) to send the shutdown signal to the child processes instead of signals. But I don't think there's an existing channel for this.

alangenfeld · 2025-02-10T16:52:15Z

the code servers can get shutdown with this i think
https://github.com/dagster-io/dagster/blob/master/python_modules/dagster/dagster/_grpc/proxy_server.py#L317

## Summary & Motivation Our only `dagster dev` tests currently live in the daemon integration test suite, which isn't run on windows. This moves them to `dagster`, which is tested on windows. The tests are mostly moved over as they were, with a few adjustments: - Instead of loading the toys repo, we now just load the simple test repo loaded in the rest of the tests. The toys repo adds a bunch of dependencies but loading it specifically doesn't really test anything relevant. - All tests have been augmented to ensure all child processes are shut down (on Unix at least-- see below) - Some refactoring to make the tests simpler to read. Previously we weren't testing this on Windows at all, and I went in thinking we would end with exactly the same tests passing on windows and unix. Unfortunately, that's not _quite_ true-- with our current set up I could not find a way to reliably and cleanly shut down the `dagster dev` process tree on Windows in a CI environment. This has to do with the differences in signal handling and process groups in Windows vs unix. I will quote ChatGPT-o1: >On Windows, CTRL_BREAK_EVENT (and similarly CTRL_C_EVENT) only works when the calling process and the target processes share a console. By default, Python’s `subprocess.Popen` with CREATE_NEW_PROCESS_GROUP spawns the child into its own new process group but does not automatically attach it to the same console as the parent (and in many CI environments, there may be no real console at all). As a result, sending CTRL_BREAK_EVENT to that child process group essentially does nothing—Windows never delivers the interrupt to those child processes, and they keep running. The lack of console in the CI environment means that `CTRL_BREAK_EVENT`, which we use to shut down the child processes, does not propagate directly from `dev` to the webserver/daemon (but it does appear to work when `dagster dev` is used interactively, due to sharing a console). So in the Windows tests I ended up explicitly killing all child processes. We could maybe make this more robust in the future by having the `dev` process send a shutdown signal via another form of IPC, but I couldn't see how to do that here. ## How I Tested These Changes Ran moved `dagster dev` tests on windows and unix.

smackesey force-pushed the windows/sean/move-dagster-dev-tests-to-core branch 5 times, most recently from 20c3b36 to 6250a23 Compare February 8, 2025 16:52

smackesey mentioned this pull request Feb 8, 2025

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

Merged

smackesey force-pushed the windows/sean/move-dagster-dev-tests-to-core branch 17 times, most recently from 8841273 to e2d78a6 Compare February 10, 2025 04:24

smackesey requested review from alangenfeld and gibsondan February 10, 2025 05:11

smackesey marked this pull request as ready for review February 10, 2025 05:11

smackesey force-pushed the windows/sean/move-dagster-dev-tests-to-core branch from e2d78a6 to d3c450d Compare February 10, 2025 13:37

smackesey mentioned this pull request Feb 10, 2025

[components] Add --json flag for dg component-type list #27710

Merged

alangenfeld reviewed Feb 10, 2025

View reviewed changes

move dagster dev tests to core

f5a7070

smackesey force-pushed the windows/sean/move-dagster-dev-tests-to-core branch from d3c450d to f5a7070 Compare February 10, 2025 16:52

smackesey mentioned this pull request Feb 10, 2025

[components] Better error message for failing entry points (BUILD-647) #27530

Open

smackesey requested a review from alangenfeld February 10, 2025 22:28

alangenfeld approved these changes Feb 10, 2025

View reviewed changes

smackesey merged commit c43dd21 into master Feb 10, 2025
13 of 14 checks passed

smackesey deleted the windows/sean/move-dagster-dev-tests-to-core branch February 10, 2025 22:52

This was referenced Feb 12, 2025

[components] Add --require-local-venv to dagster-dg #27799

Merged

[components] Reenable dg dev deployment test on windows #27815

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move dagster dev tests to core #27654

move dagster dev tests to core #27654

smackesey commented Feb 7, 2025 •

edited

Loading

smackesey commented Feb 7, 2025 •

edited

Loading

alangenfeld left a comment

alangenfeld Feb 10, 2025

alangenfeld Feb 10, 2025

alangenfeld Feb 10, 2025

smackesey Feb 10, 2025 •

edited

Loading

alangenfeld Feb 10, 2025

smackesey commented Feb 10, 2025

alangenfeld commented Feb 10, 2025

		@@ -211,6 +211,14 @@ def interrupt_ipc_subprocess(proc: "Popen[Any]") -> None:
		proc.send_signal(signal.SIGINT)


		def interrupt_or_kill_ipc_subprocess(proc: "Popen[Any]", wait_time: int = 10) -> None:

		# Skip windows here-- it behaves strangely with respect to child processes and cmdline(). But we
		# are still checking that all child processes shut down later.

move dagster dev tests to core #27654

move dagster dev tests to core #27654

Conversation

smackesey commented Feb 7, 2025 • edited Loading

Summary & Motivation

How I Tested These Changes

smackesey commented Feb 7, 2025 • edited Loading

alangenfeld left a comment

Choose a reason for hiding this comment

alangenfeld Feb 10, 2025

Choose a reason for hiding this comment

alangenfeld Feb 10, 2025

Choose a reason for hiding this comment

alangenfeld Feb 10, 2025

Choose a reason for hiding this comment

smackesey Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

alangenfeld Feb 10, 2025

Choose a reason for hiding this comment

smackesey commented Feb 10, 2025

alangenfeld commented Feb 10, 2025

smackesey commented Feb 7, 2025 •

edited

Loading

smackesey commented Feb 7, 2025 •

edited

Loading

smackesey Feb 10, 2025 •

edited

Loading