Skip to content

Conversation

@SeungjinYang
Copy link
Collaborator

Tested (run the relevant ones):

  • Code formatting: install pre-commit (auto-check on commit) or bash format.sh
  • Any manual or new tests for this PR (please specify below)
  • All smoke tests: /smoke-test (CI) or pytest tests/test_smoke.py (local)
  • Relevant individual tests: /smoke-test -k test_name (CI) or pytest tests/test_smoke.py::test_name (local)
  • Backward compatibility: /quicktest-core (CI) or pytest tests/smoke_tests/test_backward_compat.py (local)

@SeungjinYang SeungjinYang force-pushed the req-filelock-async-check branch 3 times, most recently from a6eb9fe to afa68b5 Compare October 27, 2025 19:58
@SeungjinYang SeungjinYang changed the title asyncio check warn if sync filelock is being used in async codepath Oct 27, 2025
@cg505
Copy link
Collaborator

cg505 commented Oct 27, 2025

Can we make sure that SKYPILOT_DEV is set in buildkite and then just run every smoke test ever on this

@SeungjinYang SeungjinYang changed the title warn if sync filelock is being used in async codepath fail in developer mode if sync filelock is being used in async codepath Oct 27, 2025
@SeungjinYang SeungjinYang force-pushed the req-filelock-async-check branch from a24fbdf to 1369269 Compare October 27, 2025 23:56
Copy link
Collaborator

@cg505 cg505 left a comment

Choose a reason for hiding this comment

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

I was thinking more like we wrap the FileLock class itself and only use the wrapped version, otherwise we have to remember to add this at every callsite. If you are worried about the overhead you could also do something like:

class WrappedFileLock:
    ...

if env_options.Options.IS_DEVELOPER.get():
    FileLock = WrappedFileLock
else:
    FileLock = filelock.FileLock

"""Get and update a SkyPilot API request."""
# In developer mode, we assert if synchronous filelock is
# being used in an async context.
if (env_options.Options.IS_DEVELOPER.get() and not _is_running_pytest() and
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not _is_running_pytest?

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