[Refactor] Use the sdk container for PICS file parsing - #345
Conversation
The purpose of this PR is to reduce the number of places that we parse PICS files. A forthcoming PR for the CLI will also remove parsing there in favor of calling the backend API. Eventually, this should make it easier to support parsing different types of PICS files, like zipped archives, without reduplicated code in 3 different place
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the PICS XML parsing logic to run inside the SDK container instead of parsing it locally. It introduces asynchronous parsing, updates the SDKContainer to support async context manager usage, and updates file copying methods in the container manager. The review feedback highlights several critical issues: a concurrency bug in the SDKContainer context manager due to its singleton nature, a race condition caused by a hardcoded temporary file path in PICSParser.parse, a potential TypeError when writing text-mode files, a potential AttributeError when handling command results, and an issue in copy_file_to_container where custom destination filenames are ignored during archiving.
|
I have a little bit of doubt about a couple of things, though, namely: could there be a simpler way to do this without touching the sdk repo? I think ideally, we expose the functionality a little better than what I've got here, which is loading a very specific python path and passing raw strings around, but I tried to restrict my changes to the TH side. Another issue that's nagging me is that this makes verification of PICS parsing on the TH end much harder, since it would now require the th-sdk image (that's why the tests are failing). On the one hand, we shouldn't need to anymore, since all the logic will now live in one, more easily-maintained place, but on the other hand, having endpoints without a test of their full functionality is less than ideal. We could pull in the th-sdk image as part of the backend tests, but it seems a lot like overkill. Perhaps this pushes us in the direction Cecille has been hoping to go where the python parts of the sdk image are a little less "baked in" and could be more accessible than two docker layers deep. |
|
Tick the box to add this pull request to the merge queue (same as
|
oxesoft
left a comment
oxesoft
left a comment
There was a problem hiding this comment.
Follow-up review after the AI-assisted fixup commits. Two of gemini-code-assist's earlier findings are now fully resolved (arcname bug, str/bytes TypeError). Leaving new comments below: one where a previously-flagged issue was only partially fixed, one where the mitigation could be tightened, and several new findings (CI is currently failing on Flake8/Mypy/Backend Tests on this head commit).
|
|
||
| TMP_PICS_PATH = Path(PICS_FILE_PATH + f"{id(file)}.xml") | ||
|
|
||
| sdk_container = SDKContainer() |
There was a problem hiding this comment.
Follow-up on the earlier singleton-concurrency comment on this PR (originally raised against the async with SDKContainer() version): removing __aenter__/__aexit__ didn't remove the underlying issue, it just moved it here. SDKContainer is a process-wide Singleton, and destroy() unconditionally kills/removes whatever container the singleton currently holds. If a PICS upload runs concurrently with anything else using the same container (another upload, a test execution, _generate_all_test_files()), this finally block will tear down the container out from under the other caller, which will then fail with SDKContainerNotRunning.
Since start() already treats "already running" as a no-op, parse() has no way to know whether it owns the container it's about to destroy. Consider only destroying if this call actually started the container (e.g. track was_running = sdk_container.is_running() before start(), skip destroy() if it was already True), or move to real reference counting if concurrent use is expected.
| ) | ||
| from test_collections.matter.sdk_tests.support.sdk_container import SDKContainer | ||
|
|
||
| TMP_PICS_PATH = Path(PICS_FILE_PATH + f"{id(file)}.xml") |
There was a problem hiding this comment.
Follow-up on the earlier hardcoded-temp-path race condition comment: using id(file) instead of a static path is an improvement, but id() isn't a reliable uniqueness guarantee — it's a memory address that can be reused once the earlier file object is garbage collected, and gives no protection across multiple backend processes/workers sharing the same host path. Since this is a security/correctness-sensitive path (project PICS data), uuid.uuid4() would give an actual uniqueness guarantee instead of a probabilistic one.
| raise PICSError("Parser failed to find root tag") | ||
| return root_element | ||
| async def parse(cls, file: IO) -> PICSCluster: | ||
| """Parse PICS XML using the sdk container by getting the command string from parse_pics_command""" |
There was a problem hiding this comment.
This line (and lines 61-62) currently fail Flake8 E501 (line too long, max 88) on this PR's CI run. Also, parse_pics_command is typed to take tmp_file_name: str but is called here with TMP_PICS_PATH, a Path — that's a Mypy error on this head commit as well. Either type the parameter as Path or pass str(TMP_PICS_PATH).
| raise PICSError( | ||
| f"Parser failed to read file: {result.output.decode('utf-8')}" | ||
| ) | ||
| output: str = result.output.decode("utf-8") |
There was a problem hiding this comment.
result.output is typed Union[Generator, bytes, tuple] on ExecResultExtended, but .decode("utf-8") is called on it here and on line 87 without narrowing — Mypy is currently failing on both of these on this PR's CI run. Since send_command is called here with is_stream/is_socket both left at their default False, output will in practice be bytes, but the type needs a cast/assert (or a narrower return type on the non-streaming path) to satisfy Mypy and to guard against a future caller passing stream=True.
| raise PICSError( | ||
| f"Parser failed to read file: {result.output.decode('utf-8')}" | ||
| ) | ||
| output: str = result.output.decode("utf-8") |
There was a problem hiding this comment.
The Backend Tests job is failing on this head commit. Root cause looks like app/tests/sdk_container_mock.py mocking send_command to return Mock(exit_code=0, output="mocked output") — a plain str. .decode("utf-8") on a str raises AttributeError, so any test that reaches this code path (including this PR's own updated test_pics_parser.py) blows up instead of returning/raising as expected. The mock's output needs to be bytes (e.g. b"mocked output"), or ideally a valid JSON-encoded PICS dict so test_pics_parser actually exercises the new parsing path meaningfully.
| item_number = cls.__text_for_element_child(element, "itemNumber") | ||
| return PICSItem(number=item_number, enabled=is_supported) | ||
| # cleanup | ||
| TMP_PICS_PATH.unlink(missing_ok=True) |
There was a problem hiding this comment.
If send_command raises before returning (rather than returning a non-zero exit code), this unlink() is skipped since it's not in a finally, leaking the temp file on the host. Worth moving the cleanup into a finally alongside (or nested inside) the try block.
| if root_element.tag not in ["clusterPICS", "generalPICS"]: | ||
| raise PICSError("Parser failed to find root tag") | ||
| return root_element | ||
| async def parse(cls, file: IO) -> PICSCluster: |
There was a problem hiding this comment.
Design-level note (not blocking, but worth capturing in the PR discussion since it seems related to the concerns raised in Steven's top-level comment): every PICS upload now spins up a full SDK Docker container (creation + readiness wait, up to container_bring_up_timeout=5s) to parse one small XML file that was previously parsed in-process, near-instantly. For projects that upload PICS per-cluster, project setup could become significantly slower and put repeated load on the shared container. Might be worth batching/reusing a container across multiple PICS uploads in a single project-setup flow, if that's a realistic usage pattern.
2 participants
The purpose of this PR is to reduce the number of places that we parse PICS files. A forthcoming PR for the CLI will also remove parsing there in favor of calling the backend API. Eventually, this should make it easier to support parsing different types of PICS files, like zipped archives, without reduplicated code in 3 different place