SCRF-1630: Refactor RF webapi for new workflow #2978
Conversation
momchil-flex
changed the title
dmarek-flex
left a comment
dmarek-flex
left a comment
There was a problem hiding this comment.
All your changes to TCM seem good to me
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # TODO: add support for pay_type and priority arguments, currently ignored if provided | ||
| if protocol_version is None: | ||
| protocol_version = _get_protocol_version() | ||
| return http.post( | ||
| f"tidy3d/projects/{self.batch_id}/postprocess", | ||
| f"rf/task/{self.batch_id}/submit", | ||
| { | ||
| "batchType": batch_type, | ||
| "solverVersion": solver_version, | ||
| "protocolVersion": protocol_version, | ||
| "workerGroup": worker_group, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
logic: pay_type and priority parameters are accepted but not actually sent to the API (see line 849-856). However, webapi.start() at line 722-727 passes these parameters expecting them to work for batch tasks, which could lead to silent failures where users think they've set priority/payment but haven't.
Either implement support for these parameters in the API call or raise an error if they're provided for batch tasks.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tidy3d/web/core/task_core.py
Line: 846:856
Comment:
**logic:** `pay_type` and `priority` parameters are accepted but not actually sent to the API (see line 849-856). However, `webapi.start()` at line 722-727 passes these parameters expecting them to work for batch tasks, which could lead to silent failures where users think they've set priority/payment but haven't.
Either implement support for these parameters in the API call or raise an error if they're provided for batch tasks.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Pull Request Overview
This PR refactors the RF (component modeler) web API to align with a new backend workflow, simplifying the task lifecycle management and removing manual postprocessing steps.
- Removes multi-phase task workflows (validate, run, postprocess) in favor of a unified flow
- Updates RF task creation to use new
rf/taskendpoints instead of legacytidy3d/projectsendpoints - Eliminates manual postprocessing orchestration, now handled automatically by the backend
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| tidy3d/web/core/task_info.py | Removes TaskStatus and BatchStatus enums; adds taskType field to BatchDetail and removes deprecated totalStatus field |
| tidy3d/web/core/task_core.py | Refactors task creation to use RF-specific endpoints; removes wait_for_validate, wait_for_run, and postprocess methods; adds abort method for batch tasks |
| tidy3d/web/core/http_util.py | Temporarily hardcodes version string (should be reverted before production) |
| tidy3d/web/api/webapi.py | Simplifies batch monitoring by removing manual postprocessing logic; updates status checking and error handling for new workflow; removes postprocess_start function |
| tidy3d/web/api/tidy3d_stub.py | Generalizes validation to use else instead of explicit EMESimulation check |
| tidy3d/web/api/states.py | Updates error state names to match new backend conventions (e.g., validate_error instead of validate_fail) |
| tidy3d/web/api/container.py | Removes postprocess_status property and postprocess_start methods from Job and Batch classes |
| tidy3d/web/init.py | Removes postprocess_start from public API exports |
| tidy3d/plugins/smatrix/component_modelers/terminal.py | Adds validate=False to intermediate simulation copies for performance; moves grid checks to base simulation; adds validate_pre_upload method |
| tidy3d/plugins/smatrix/component_modelers/base.py | Adds validate_pre_upload method to base class |
| tidy3d/config/profiles.py | Updates dev profile endpoints to new infrastructure |
| tidy3d/components/mode/simulation.py | Removes source_required parameter from validate_pre_upload signature |
| tidy3d/components/mode/mode_solver.py | Removes source_required parameter from validate_pre_upload signature |
| docs/api/submit_simulations.rst | Removes TaskStatus from documentation |
Comments suppressed due to low confidence (1)
tidy3d/web/core/task_core.py:268
- The payload structure for RF tasks differs from non-RF tasks. The RF task uses
groupNameandfolderIddirectly while non-RF tasks usetaskName. However, line 266 suggests that RF tasks should return agroupIdorbatchIdas thetaskId. Ensure that the response parsing logic on lines 264-268 correctly handles both response formats and that downstream code expecting ataskIdcan work with agroupId/batchId.
if task_type == "RF":
payload = {
"groupName": task_name,
"folderId": folder.folder_id,
"fileType": file_type,
}
resp = http.post("rf/task", payload)
else:
payload = {
"taskName": task_name,
"taskType": task_type,
"callbackUrl": callback_url,
"simulationType": simulation_type,
"parentTasks": parent_tasks,
"fileType": file_type,
}
resp = http.post(f"{projects_endpoint}/{folder.folder_id}/tasks", payload)
# RF group creation may return group-level info without 'taskId'.
# Use 'groupId' (or 'batchId' as fallback) as the resource id for subsequent uploads.
if "taskId" not in resp and task_type == "RF":
# Prefer using 'batchId' as the resource id for uploads (S3 STS expects a task-like id).
if "batchId" in resp:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| detail = batch.detail() | ||
| status = detail.status.lower() | ||
| if status in ERROR_STATES: | ||
| _batch_detail_error(task_id) |
There was a problem hiding this comment.
The _batch_detail_error call on line 758 doesn't return its result. This means the error will be logged internally but not raised. The pattern should be raise _batch_detail_error(task_id) or the function should handle the error raising internally and this call should just trigger it.
| _batch_detail_error(task_id) | |
| raise _batch_detail_error(task_id) |
| return False | ||
|
|
||
| def detail(self, batch_type: str) -> BatchDetail: | ||
| def detail(self) -> BatchDetail: |
There was a problem hiding this comment.
Missing return type annotation. The function should specify that it returns BatchDetail or None.
| status in ERROR_STATES | ||
| and hasattr(batch_detail, "validateErrors") |
There was a problem hiding this comment.
The error handling logic appears inconsistent. On line 117, batch_detail.status.lower() is called, but if status is a string already, this should work. However, the logic on line 126 checks if status in ERROR_STATES, which should be checking the actual status condition for raising errors. The current condition seems redundant with the check on line 122.
| status in ERROR_STATES | |
| and hasattr(batch_detail, "validateErrors") | |
| hasattr(batch_detail, "validateErrors") |
| # TODO: seems like the default should then be maybe set to None and defined somewhere else | ||
| # per task type? |
There was a problem hiding this comment.
[nitpick] The TODO comment suggests the default path handling logic should be refactored to be task-type specific rather than hardcoded here. Consider extracting this logic into a task-type configuration or factory pattern for better maintainability.
| else: | ||
| self.simulation.validate_pre_upload() | ||
|
|
There was a problem hiding this comment.
[nitpick] The change from elif to else removes the explicit type check for EMESimulation. If other simulation types are added in the future, they will automatically call validate_pre_upload() without the source_required parameter. Consider whether this should remain an elif isinstance(self.simulation, EMESimulation): check for explicitness and to catch unexpected simulation types.
| else: | |
| self.simulation.validate_pre_upload() | |
| elif isinstance(self.simulation, EMESimulation): | |
| self.simulation.validate_pre_upload() | |
| else: | |
| raise TypeError(f"Unexpected simulation type: {type(self.simulation).__name__}") |
| return file | ||
|
|
||
| def abort(self) -> requests.Response: | ||
| """Aborting current task from server.""" |
There was a problem hiding this comment.
Error message has inconsistent grammar: "Aborting current task from server" should be "Aborting current task on server" or "Aborting task from server".
| """Aborting current task from server.""" | |
| """Abort the current task on the server.""" |
| return ("validating", 2) | ||
| if s in ("validate_success", "validate_warn"): | ||
| return ("validate", 3) | ||
| if s in ("running", "preprocess_success"): |
There was a problem hiding this comment.
The mapping for "preprocess_success" on line 1249 returns stage ("running", 4), but this seems inconsistent. If preprocessing succeeded, the task should be at the "preprocess" stage (stage 1) or moving to validation (stage 2), not "running". This mapping should be reviewed for correctness.
| if s in ("running", "preprocess_success"): | |
| if s == "preprocess_success": | |
| return ("validating", 2) | |
| if s == "running": |
| "workerGroup": worker_group, | ||
| }, | ||
| ) | ||
|
|
There was a problem hiding this comment.
The wait_for_validate and wait_for_run methods have been removed from the BatchTask class. If any code in the codebase depends on these methods, this will cause AttributeError exceptions. Verify that all usages of these methods have been refactored or removed.
| def _get_batch_detail_handle_error_status(task_id: TaskId) -> str: | ||
| """Get batch detail and raise error if status is in ERROR_STATES.""" | ||
| batch = BatchTask(task_id) | ||
| detail = batch.detail() | ||
| status = detail.status.lower() | ||
| if status in ERROR_STATES: | ||
| _batch_detail_error(task_id) | ||
| return detail |
There was a problem hiding this comment.
The function _get_batch_detail_handle_error_status has a return type annotation of str, but it returns a BatchDetail object on line 759. This will cause type checking issues and the function is used on line 771 as if it returns a BatchDetail (accessing .status). The return type should be BatchDetail.
| # return core_config.get_version() | ||
| return "2.10.0rc2.1" |
There was a problem hiding this comment.
The hardcoded version string "2.10.0rc2.1" appears to be a temporary change for testing. This should be reverted to use core_config.get_version() before merging to production.
Greptile Overview
Greptile Summary
This PR refactors the RF webapi to use a new simplified workflow for Component Modeler batch tasks. The changes migrate from legacy
tidy3d/tasks/{id}/batch-*endpoints to newrf/task/{id}/*endpoints, removing ~400 lines of complex async split/validation logic.Key Changes
_upload_component_modeler_subtasks()function that handled async job splitting and multi-phase validation (~170 lines)BatchTasknow uses consistentrf/task/*endpoints for check, submit, detail, and statisticsJobandBatchclassesPOST rf/task(new) vsPOST tidy3d/projects/{folder}/tasks(old)GET rf/task/{id}/statistics(new) vsGET tidy3d/tasks/{id}/batch-detail(old)POST rf/task/{id}/submit(new) vsPOST tidy3d/projects/{id}/batch-submit(old)validate=Falseinupdated_copy()calls to defer validation until final assemblyBatchTask.wait_for_validate(),BatchTask.wait_for_run(),BatchTask.postprocess(),Job.postprocess_start(),Batch.postprocess_start()Potential Issues
BatchTask.submit()acceptspay_typeandpriorityparameters but doesn't send them to the API (line 846-856 intask_core.py). This could cause silent failures where users believe they've set priority/payment method but haven't.Confidence Score: 3/5
pay_type/priorityparameters are accepted but not sent to API for batch tasks, potentially causing silent failures. (2) Large refactor (~400 lines removed) migrating to new endpoints with limited visibility into backend changes. (3) Validation deferral in component modelers changes error detection timing. However, the refactor itself appears well-structured with consistent patterns and removes complex async logic.tidy3d/web/core/task_core.py(BatchTask.submit method) - fix the ignored parameters issueImportant Files Changed
File Analysis
validate=Falseto simulation.updated_copy() calls to defer validation, moved grid size checks earlier in workflowSequence Diagram
sequenceDiagram participant User participant webapi participant SimulationTask participant BatchTask participant API as RF API (rf/task/*) User->>webapi: upload(simulation) webapi->>webapi: Check if RF task type alt RF Task (Component Modeler) webapi->>SimulationTask: create(task_type="RF") SimulationTask->>API: POST rf/task (create group) API-->>SimulationTask: batchId/groupId SimulationTask->>API: Upload simulation data to S3 webapi->>webapi: estimate_cost(resource_id) else Regular Task webapi->>SimulationTask: create(task_type) SimulationTask->>API: POST tidy3d/projects/{folder}/tasks API-->>SimulationTask: taskId SimulationTask->>API: Upload simulation data to S3 end User->>webapi: start(task_id) webapi->>webapi: Check if modeler batch alt Batch Task webapi->>BatchTask: new(batch_id) BatchTask->>API: POST rf/task/{id}/submit Note right of API: pay_type/priority<br/>NOT sent (known issue) else Regular Task webapi->>SimulationTask: get(task_id) SimulationTask->>API: POST tidy3d/tasks/{id}/submit end User->>webapi: monitor(task_id) loop Poll Status alt Batch Task webapi->>BatchTask: detail() BatchTask->>API: GET rf/task/{id}/statistics API-->>BatchTask: BatchDetail else Regular Task webapi->>SimulationTask: get_info() SimulationTask->>API: GET tidy3d/tasks/{id}/detail API-->>SimulationTask: TaskInfo end end User->>webapi: download(task_id) alt Batch Task webapi->>BatchTask: get_data_hdf5() BatchTask->>API: Download from S3 else Regular Task webapi->>SimulationTask: get_sim_data_hdf5() SimulationTask->>API: Download from S3 end