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

Use a dev drive for testing on Windows #15664

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Use a dev drive for testing on Windows #15664

wants to merge 6 commits into from

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Jan 21, 2025

Investigating the benefits of this here.

Requires Windows 2025, switching over in #15661

@zanieb zanieb added the internal An internal refactor or improvement label Jan 21, 2025
Copy link
Contributor

github-actions bot commented Jan 21, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@zanieb zanieb force-pushed the zb/dev-drive branch 3 times, most recently from 47662ec to 322c5ab Compare January 22, 2025 00:04
@zanieb
Copy link
Member Author

zanieb commented Jan 22, 2025

Hm okay this gives a big speed-up but a test is failing

──── STDERR:             red_knot::file_watching directory_deleted
thread 'directory_deleted' panicked at /rustc/9fc6b43126469e3858e2fe86cafb4f0fd5068869\library\core\src\ops\function.rs:250:5:
Didn't observe expected change:
  - Deleted { path: "E:\\ruff-tmp\\.tmppcH4p4\\project\\sub\\a.py", kind: Any }
  - Deleted { path: "E:\\ruff-tmp\\.tmppcH4p4\\project\\sub\\__init__.py", kind: Any }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@zanieb
Copy link
Member Author

zanieb commented Jan 22, 2025

I think the small runner on the D:\ drive is too slow at ~12 minutes.

@MichaReiser would you be interested in investigating if this is a real bug with Dev Drives? I'm not sure if there are known issues with the file watcher?

I'm happy to procure more logs if you tell me how. Or I can test a minimal snippet, if you can extract the core folder-watching behavior or point me to it?

@zanieb zanieb added ci Related to internal CI tooling and removed internal An internal refactor or improvement labels Jan 22, 2025
@@ -0,0 +1,62 @@
# Configures a drive for testing in CI.
Copy link
Member Author

Choose a reason for hiding this comment

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

Scary, but just copied over from uv.

@zanieb
Copy link
Member Author

zanieb commented Jan 22, 2025

Glorious, here are the logs

──── STDERR:             red_knot::file_watching directory_deleted
2025-01-22T16:57:43.337130Z DEBUG red_knot_project::metadata: Searching for a project in 'E:\ruff-tmp\.tmpLgedMu\project'
2025-01-22T16:57:43.337503Z DEBUG red_knot_project::metadata: The ancestor directories contain no `pyproject.toml`. Falling back to a virtual project.
2025-01-22T16:57:43.338307Z  INFO red_knot_python_semantic::program: Python version: Python 3.9, platform: all
2025-01-22T16:57:43.338388Z DEBUG red_knot_python_semantic::module_resolver::resolver: Adding first-party search path 'E:\ruff-tmp\.tmpLgedMu\project'
2025-01-22T16:57:43.338468Z DEBUG red_knot_python_semantic::module_resolver::resolver: Using vendored stdlib
2025-01-22T16:57:43.345608Z TRACE red_knot_project::db: Salsa event: Event { thread_id: ThreadId(2), kind: WillExecute { database_key: dynamic_resolution_paths(Id(800)) } }
2025-01-22T16:57:43.345729Z DEBUG red_knot_python_semantic::module_resolver::resolver: Resolving dynamic module resolution paths
2025-01-22T16:57:43.345984Z DEBUG red_knot_project::watch::watcher: Watching path: `E:\ruff-tmp\.tmpLgedMu\project`
2025-01-22T16:57:43.346325Z  INFO red_knot_project::watch::project_watcher: Set up file watchers for ["E:\ruff-tmp\.tmpLgedMu\project"]
2025-01-22T16:57:43.448406Z TRACE ruff_db::files: Adding file 'E:\ruff-tmp\.tmpLgedMu\project\bar.py'
2025-01-22T16:57:43.449023Z TRACE red_knot_project::db: Salsa event: Event { thread_id: ThreadId(2), kind: WillExecute { database_key: resolve_module_query(Id(1000)) } }
2025-01-22T16:57:43.449297Z TRACE resolve_module: ruff_db::files: Adding file 'E:\ruff-tmp\.tmpLgedMu\project\sub\__init__.py' name=sub.a
2025-01-22T16:57:43.449472Z TRACE resolve_module: ruff_db::files: Adding file 'E:\ruff-tmp\.tmpLgedMu\project\sub\a\__init__.pyi' name=sub.a
2025-01-22T16:57:43.449590Z TRACE resolve_module: ruff_db::files: Adding file 'E:\ruff-tmp\.tmpLgedMu\project\sub\a\__init__.py' name=sub.a
2025-01-22T16:57:43.449696Z TRACE resolve_module: ruff_db::files: Adding file 'E:\ruff-tmp\.tmpLgedMu\project\sub\a.pyi' name=sub.a
2025-01-22T16:57:43.449789Z TRACE resolve_module: ruff_db::files: Adding file 'E:\ruff-tmp\.tmpLgedMu\project\sub\a.py' name=sub.a
2025-01-22T16:57:43.449905Z TRACE resolve_module: red_knot_python_semantic::module_resolver::resolver: Resolved module `sub.a` to `E:\ruff-tmp\.tmpLgedMu\project\sub\a.py` name=sub.a
2025-01-22T16:57:43.455836Z  INFO Project::index_files: red_knot_project: Found 3 files in project `project` package=project
thread 'directory_deleted' panicked at /rustc/9fc6b43126469e3858e2fe86cafb4f0fd5068869\library\core\src\ops\function.rs:250:5:
Didn't observe expected change:
  - Deleted { path: "E:\\ruff-tmp\\.tmpLgedMu\\project\\sub\\a.py", kind: Any }
  - Deleted { path: "E:\\ruff-tmp\\.tmpLgedMu\\project\\sub\\__init__.py", kind: Any }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@zanieb
Copy link
Member Author

zanieb commented Jan 22, 2025

(they don't seem helpful — not sure how to get more...)

@MichaReiser
Copy link
Member

(they don't seem helpful — not sure how to get more...)

How dare you! I improved them a lot and I think there's some useful content in them.

The specific test tests if the file watcher correctly handles the case where an entire directory gets deleted, specifically the sub directory. That's why the test expects a change event for the sub directory. What's interesting is that no such event gets emitted on the devdrive. It only emits the events for the files in that directory

Didn't observe expected change:
  - Deleted { path: "E:\\ruff-tmp\\.tmpLgedMu\\project\\sub\\a.py", kind: Any }
  - Deleted { path: "E:\\ruff-tmp\\.tmpLgedMu\\project\\sub\\__init__.py", kind: Any }

Now, we could fix the test to expect the events for one of the files instead of the directories, and I suspect it would then pass just fine on all platforms.

However, this reveals a more fundamental problem with changing to dev drives: Windows's file-watching API returns different events depending on whether the path is a dev drive or not. Migrating to a dev drive would, therefore, remove the coverage for regular Windows drives, which I consider fairly important.

That means I'm not sure if we should change our runner to use dev drivers. Is there a way to only use a dev drive for some directories, e.g., not the directory created by tempdir?

Comment on lines +244 to +245
RED_KNOT_LOG: debug
RUST_LOG: debug
Copy link
Member

Choose a reason for hiding this comment

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

This should not be needed when you use testing::setup_tracing() It always writes the logs and they should be visible if the test fails

} else {
# The size (20 GB) is chosen empirically to be large enough for our
# workflows; larger drives can take longer to set up.
$Volume = New-VHD -Path C:/ruff_dev_drive.vhdx -SizeBytes 20GB |
Copy link
Member

Choose a reason for hiding this comment

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

We can probably get away with a smaller drive for Ruff.


# Note we use `Get-PSDrive` is not sufficient because the drive letter is assigned.
if (Test-Path "D:\") {
Write-Output "Using existing drive at D:"
Copy link
Member

Choose a reason for hiding this comment

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

A beautiful mix of space and tab indentation. hehe

@zanieb
Copy link
Member Author

zanieb commented Jan 24, 2025

Funny, I read

Didn't observe expected change:
  - Deleted { path: "E:\\ruff-tmp\\.tmpLgedMu\\project\\sub\\a.py", kind: Any }
  - Deleted { path: "E:\\ruff-tmp\\.tmpLgedMu\\project\\sub\\__init__.py", kind: Any }

as: we expected these changes but did not observe them.

Now I see — the message is much more helpful read that way.

@zanieb
Copy link
Member Author

zanieb commented Jan 24, 2025

However, this reveals a more fundamental problem with changing to dev drives: Windows's file-watching API returns different events depending on whether the path is a dev drive or not. Migrating to a dev drive would, therefore, remove the coverage for regular Windows drives, which I consider fairly important.

Yeah I agree coverage for a regular drive seems important. It sounds like it'd actually be important to get both coverage for a Dev Drive and the regular file system. I can own that.

That means I'm not sure if we should change our runner to use dev drivers. Is there a way to only use a dev drive for some directories, e.g., not the directory created by tempdir?

Yeah, but then we'll probably miss out on the benefits. I can explore.

@MichaReiser
Copy link
Member

That's fair. There's definetely still room for improvement. Ideally, the message would also say which event we expected. But it's much better than in the past where it just told you: Too bad, didn't work 😆

@zanieb
Copy link
Member Author

zanieb commented Jan 24, 2025

Thanks for taking a look Micha. I see this as relatively low priority, but I can explore it in my spare time,

@MichaReiser
Copy link
Member

Thank you. Excluding the temp directory might be fine for Ruff. We don't write as many files as you do in uv. It's only a handful tests that make use of temp (although I don't know if cargo or other tools make heavy use of temp)

zanieb added a commit that referenced this pull request Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Related to internal CI tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants