Repository navigation
feat/uploader thread split - #1523
Conversation
Introduce a lightweight Timer scheduler (swanlab.sdk.internal.pkg.timer) implemented with a daemon thread, supporting fixed or dynamic intervals, optional immediate execution, cooperative cancel(), restartable start()/run() (logs a warning if already running), join(), execution_count and is_running properties, and defensive interval validation. The Timer logs task and strategy errors via console and ensures the running task is not forcibly killed. Add comprehensive unit tests covering basic scheduling, immediate run, dynamic interval strategy, cancel interrupting sleep, error resilience, double-run warning, restart behavior, and invalid interval rejection, plus a fixture that cleans up timers after each test.
![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 introduces a background uploading mechanism for SwanLab records, including a new Uploader component, a timer-based task scheduler, and a factory for core implementations. It also adds a cache cleaning script and associated unit tests. Feedback focuses on improving thread safety and efficiency in the Collector by replacing boolean flags and busy-waiting with proper locking mechanisms. Additionally, a bug was identified where the upload_interval parameter was ignored in the Uploader constructor, and several unit tests require corrections to their mock patch targets. Suggestions were also made to optimize the cache cleaning script and make the upload rate-limiting more flexible.
Introduce grouping of protobuf Records by their record_type (helper.group_records_by_type) and change trace_records to batch by type and call a new upload_record_group transport API. Add an HTTP transport skeleton (HttpRecordTransport) that dispatches record groups to typed handlers and keep a NoopRecordTransport for disabled mode; create_record_transport now returns the HTTP skeleton when enabled. Adjust CoreTransportConfig default enabled behavior and make ThreadPool.upload_interval optional with a resolved default. Update tests and imports to reflect grouping, new transport methods, and protobuf timestamp import fixes.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an asynchronous record uploader for the SwanLab SDK, featuring a background thread pool for batching and uploading protobuf records. It also implements a factory pattern for core components and a new lightweight timer utility for periodic tasks. A review comment identifies a potential concurrency bottleneck in the Collector class, where network I/O is performed while holding a lock, which could block the main thread during shutdown.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a background uploader for the SwanLab SDK, enabling asynchronous record transmission to the cloud. Key additions include a Core factory for abstracting core implementations, an Uploader component for managing background tasks, and a lightweight Timer module. Additionally, a cache cleaning script and corresponding Makefile target were added. Feedback focuses on improving upload performance by removing a hardcoded sleep in the transport layer and optimizing queue operations through batching.
Correct a typo in the module docstring: changed @file from 'test_cnmd_helper.py' to 'test_cmd_helper.py' in tests/unit/sdk/cmd/test_cmd_helper.py. This is a documentation-only change with no functional impact.
Replace '聚合器' with '采集器' in the file header and class docstring of swanlab/sdk/internal/core_python/uploader/collector.py to more accurately reflect the collector terminology.
Description
Closes: