Add file watching client changes

[azliu0]

Describe your changes

WRK-484

Blockers

• https://github.com/modal-labs/modal/pull/18215 roll out to all workers

Backward/forward compatibility checks

---

Check these boxes or delete any item (or this section) if not relevant for this PR.

• Client+Server: this change is compatible with old servers
• Client forward compatibility: this change ensures client can accept data intended for later versions of itself

Note on protobuf: protobuf message changes in one place may have impact to
multiple entities (client, server, worker, database). See points above.

---

Changelog

• Sandboxes now support fsnotify-like file watching:

from modal.file_io import FileWatchEventType

app = modal.App.lookup("file-watch", create_if_missing=True)
sb = modal.Sandbox.create(app=app)
events = sb.watch("/foo")
for event in events:
    if event.type == FileWatchEventType.Modify:
        print(event.paths)

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to bb07b51 in 51 seconds

More details

• Looked at 236 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. modal/sandbox.py:590

• Draft comment:
  The watch method should return AsyncIterator[FileWatchEvent] instead of AsyncIterator[bytes] to match the _FileIO.watch method.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment suggests a change in the return type based on the assumption that _FileIO.watch returns FileWatchEvent. However, without seeing the implementation of _FileIO.watch, it's speculative. The current implementation yields bytes, which matches the declared return type. Without strong evidence from the code provided, the comment seems speculative.
  I might be missing the actual implementation of _FileIO.watch, which could indeed return FileWatchEvent. However, without this information, the comment remains speculative.
  Given the rules, I should only keep comments with strong evidence. Since the evidence is not present in the provided code, the comment should be removed.
  Remove the comment as it is speculative and lacks strong evidence from the provided code.

Workflow ID: wflow_HIajFznDcjDgbgmj

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Reviewed everything up to 26ffa5e in 3 minutes and 16 seconds

More details

• Looked at 178 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. modal/sandbox.py:575

• Draft comment:
  The watch method should yield FileWatchEvent instead of bytes to match the return type of _FileIO.watch.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment is about a change made in this diff, specifically the addition of the watch method. However, without seeing the implementation of _FileIO.watch, it's difficult to verify the correctness of the comment. The comment suggests a specific type mismatch, which could be a valid issue if _FileIO.watch indeed returns FileWatchEvent. Without more context, it's hard to be certain.
  I might be missing the implementation details of _FileIO.watch, which could confirm or refute the comment's claim. Without this information, my assessment is speculative.
  Given the lack of visibility into _FileIO.watch, it's reasonable to be cautious and consider the possibility that the comment is correct. However, without strong evidence, it's safer to err on the side of deletion.
  Delete the comment due to lack of strong evidence supporting its correctness. The implementation details of _FileIO.watch are not visible, making it speculative.

Workflow ID: wflow_EE5EQiXGr71uUl9k

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[azliu0]

@ellipsis-dev quiet

[ellipsis-dev]

It seems like your comment '@ellipsis-dev quiet' doesn't specify a request. Could you please clarify what you need assistance with?

[ellipsis-dev]

👍 Looks good to me! Incremental review on 4996316 in 10 seconds

More details

• Looked at 97 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. test/file_io_test.py:350

• Draft comment:
  Consider adding an assertion to verify that FileIO.watch is called with the correct parameters. This ensures the method is invoked as expected.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The test function test_file_watch is missing an assertion to check if the FileIO.watch method is called with the correct parameters. This is important to ensure that the method is being invoked as expected.

Workflow ID: wflow_FnXD6PRxPGnL8e8Z

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on f177980 in 1 minute and 4 seconds

More details

• Looked at 2340 lines of code in 40 files
• Skipped 0 files when reviewing.
• Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_djiNdT41139LIM6g

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 0a81a93 in 12 seconds

More details

• Looked at 31 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. test/file_io_test.py:391

• Draft comment:
  The assertion assert e.type == se.type has been updated to use event.type instead of event.event. Ensure that this change is consistent with the rest of the codebase and that event.type is the correct attribute to use.
• Reason this comment was not posted:
  Comment did not seem useful.

Workflow ID: wflow_AT0omgORV5p1gRUc

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on cc1e647 in 27 seconds

More details

• Looked at 45 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. test/file_io_test.py:384

• Draft comment:
  The watch method is asynchronous and should be awaited.

        events = await FileIO.watch("/test.txt", client, "task-123")

• Reason this comment was not posted:
  Comment was on unchanged code.

Workflow ID: wflow_NmLRJNE3vBM6n8yC

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 50e32af in 23 seconds

More details

• Looked at 47 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. test/file_io_test.py:384

• Draft comment:
  The watch method returns an AsyncIterator, so it should be awaited. Use async for to iterate over events.
• Reason this comment was not posted:
  Comment was on unchanged code.

Workflow ID: wflow_QFRUKI2E55AvFhxA

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[pawalt]

When is this not the case?

[azliu0]

when a single event is split across multiple items

[ellipsis-dev]

👍 Looks good to me! Incremental review on b9299fe in 18 seconds

More details

• Looked at 267 lines of code in 4 files
• Skipped 0 files when reviewing.
• Skipped posting 3 drafted comments based on config settings.

1. modal/io_streams_helper.py:24

• Draft comment:
  The parameter name in the docstring should be stream instead of stream_generator.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The PR introduces a new helper function consume_stream_with_retries to handle stream consumption with retry logic. This function is used in multiple places, replacing the previous retry logic. However, the docstring for consume_stream_with_retries contains an incorrect parameter name stream_generator which should be stream. This should be corrected for clarity.

2. modal/file_io.py:449

• Draft comment:
  Consider renaming the filter parameter to avoid shadowing the built-in filter function. For example, you could use event_filter.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The PR introduces a new filter parameter in the watch method of the _FileIO class to filter file watch events. However, the filter parameter name shadows the built-in filter function in Python, which can lead to confusion. It's better to use a different name for this parameter.

3. modal/io_streams_helper.py:33

• Draft comment:
  Consider adding a timeout or maximum iteration count to handle cases where the stream might be exhausted without encountering a completion item, to prevent potential infinite loops.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The PR introduces a new consume_stream_with_retries function that is used in multiple places. However, the function does not handle the case where the stream might be exhausted without encountering a completion item. This could lead to an infinite loop if the stream ends unexpectedly without a completion item. A timeout or maximum iteration count could be added to prevent this.

Workflow ID: wflow_71jrJtps3C5aEnmr

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 081458f in 13 seconds

More details

• Looked at 13 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. modal/io_streams.py:181

• Draft comment:
  The check item is not None before decoding is a good practice to avoid NoneType errors. Ensure similar checks are applied elsewhere in the code where decoding occurs.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The change in the PR adds a check for item is not None before decoding it. This is a good practice to avoid potential NoneType errors when decoding. However, the same check should be applied to other similar instances in the code.

Workflow ID: wflow_iSyE0UZO6xvmLhDi

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 79f2afe in 11 seconds

More details

• Looked at 28 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. modal/io_streams_helper.py:21

• Draft comment:
  The mdmd:hidden tag is not properly closed. It should be """mdmd:hidden""" to ensure proper documentation formatting.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The docstring for the function consume_stream_with_retries is incorrectly formatted. The mdmd:hidden tag is not properly closed, which might lead to documentation issues.

Workflow ID: wflow_DcPvqC4AlPcVBLQ9

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.