Unify batch and simulation task handling with TaskFactory pattern

[Copilot]

Addresses review feedback on PR #2978 to eliminate redundant API calls and unify task handling between SimulationTask and BatchTask.

Changes

• TaskFactory pattern: Introduced TaskFactory.get() that returns the appropriate WebTask subclass (SimulationTask or BatchTask) based on task type, with registry caching to avoid repeated API calls
• WebTask base class: Extracted common task operations (get_log(), get_data_hdf5(), get_url(), is_batch()) into shared WebTask base class
• Unified webapi functions: Refactored all webapi functions (download(), get_info(), start(), abort(), monitor(), etc.) to use TaskFactory.get() instead of _is_modeler_batch() checks, eliminating redundant HTTP calls
• Test fixture: Added clear_task_factory_registry() autouse fixture to prevent stateful test dependencies
• API coverage: Added NotImplementedError for batch-unsupported operations (get_run_info(), download_json(), load_simulation(), download_log(), delete(), download_simulation())
• Code cleanup: Removed deprecated compose_modeler() function in favor of Tidy3dBaseModel.from_file(), exposed Tidy3dBaseModel in public API

Architecture

Before:

# Multiple API calls to determine task type
if _is_modeler_batch(task_id):  # API call 1
    batch = BatchTask(task_id)
    detail = batch.detail()  # API call 2
else:
    task = SimulationTask.get(task_id)  # API call 1

After:

# Single API call with caching
task = TaskFactory.get(task_id)  # API call 1 (cached for subsequent calls)
if isinstance(task, BatchTask):
    detail = task.detail()  # API call 2

The factory maintains a registry mapping task_id → task_type to eliminate redundant type-checking API calls across webapi function calls.

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Greptile Overview

Greptile Summary

This PR introduces a TaskFactory pattern to unify handling of SimulationTask and BatchTask, eliminating redundant API calls through registry caching. The refactoring extracts common operations into a WebTask base class and updates all webapi functions to use TaskFactory.get() instead of manual type checking.

Key Changes

• TaskFactory pattern: Introduces TaskFactory.get() that returns the appropriate WebTask subclass based on task type, with registry caching to avoid repeated API calls
• WebTask base class: Extracts shared operations (get_log(), get_data_hdf5(), get_url(), is_batch()) into base class inherited by both SimulationTask and BatchTask
• Unified webapi: All webapi functions (download(), get_info(), start(), abort(), monitor()) now use TaskFactory.get() instead of _is_modeler_batch() checks
• Test fixture: Added autouse fixture to clear TaskFactory registry between tests

Issues Found

• Critical: BatchTask.submit() accepts pay_type and priority parameters but raises NotImplementedError instead of sending them to the API. This breaks the unified interface and could confuse users who expect these parameters to work (tidy3d/web/core/task_core.py:896-940)
• Inefficiency: TaskFactory.get() makes redundant HTTP calls when task not in registry - checks is_batch(), registers, then calls .get() which fetches again (tidy3d/web/core/task_core.py:978-991)
• Minor: Duplicate TaskFactory.reset() call in test fixture (tidy3d/web/tests/conftest.py:12-13)

Confidence Score: 3/5

• This PR has critical logic issues that must be resolved before merging
• Score of 3 reflects: (1) Critical bug where BatchTask.submit() accepts pay_type/priority parameters but raises NotImplementedError instead of using them, breaking API contract and potentially causing silent failures or confusing errors. (2) Inefficiency in TaskFactory.get() making redundant API calls defeats the caching optimization goal. (3) The refactoring itself is well-structured with good separation of concerns and the test fixture addition is appropriate. However, the critical parameter handling issue must be fixed.
• Pay close attention to tidy3d/web/core/task_core.py - fix the BatchTask.submit() parameter handling issue and optimize TaskFactory.get() to avoid redundant HTTP calls

Important Files Changed

File Analysis

Filename	Score	Overview
tidy3d/web/core/task_core.py	3/5	Introduced WebTask base class, TaskFactory pattern with registry caching, unified task operations. Critical issue: BatchTask.submit() accepts but doesn't send pay_type/priority to API. Inefficiency in TaskFactory.get() making redundant HTTP calls.
tidy3d/web/api/webapi.py	4/5	Refactored all webapi functions to use TaskFactory.get() instead of manual type checking, eliminating redundant API calls. Clean unified interface across download(), get_info(), start(), abort(), monitor() etc.
tidy3d/web/tests/conftest.py	4/5	Added autouse fixture to clear TaskFactory registry before/after each test. Minor issue: duplicate reset call at line 12-13.

Sequence Diagram

sequenceDiagram
    participant User
    participant webapi
    participant TaskFactory
    participant SimulationTask
    participant BatchTask
    participant API as Server API

    User->>webapi: upload(simulation)
    webapi->>WebTask: create(task_type, task_name)
    
    alt RF Task Type
        WebTask->>API: POST rf/task
        API-->>WebTask: batchId/groupId
    else Standard Task Type
        WebTask->>API: POST tidy3d/projects/{folder}/tasks
        API-->>WebTask: taskId
    end
    
    WebTask->>API: Upload simulation to S3
    webapi->>User: return task_id

    User->>webapi: start(task_id)
    webapi->>TaskFactory: get(task_id)
    
    alt task_id in registry cache
        TaskFactory-->>webapi: return cached task type
    else task_id not in registry
        TaskFactory->>API: GET rf/task/{id}/statistics
        alt is batch
            TaskFactory->>TaskFactory: register(task_id, "batch")
            TaskFactory->>BatchTask: get(task_id)
            BatchTask->>API: GET rf/task/{id}/statistics
            BatchTask-->>TaskFactory: BatchTask instance
        else is simulation
            TaskFactory->>SimulationTask: get(task_id)
            SimulationTask->>API: GET tidy3d/tasks/{id}/detail
            SimulationTask-->>TaskFactory: SimulationTask instance
            TaskFactory->>TaskFactory: register(task_id, "simulation")
        end
    end
    
    TaskFactory-->>webapi: WebTask instance
    
    alt BatchTask
        webapi->>BatchTask: submit(solver_version, worker_group)
        Note right of BatchTask: pay_type/priority NOT sent
        BatchTask->>API: POST rf/task/{id}/submit
    else SimulationTask
        webapi->>SimulationTask: submit(solver_version, worker_group, pay_type, priority)
        SimulationTask->>API: POST tidy3d/tasks/{id}/submit
    end

    User->>webapi: monitor(task_id)
    webapi->>TaskFactory: get(task_id)
    Note right of TaskFactory: Uses registry cache (no extra HTTP call)
    TaskFactory-->>webapi: WebTask instance
    
    loop Poll Status
        alt BatchTask
            webapi->>BatchTask: detail()
            BatchTask->>API: GET rf/task/{id}/statistics
        else SimulationTask
            webapi->>SimulationTask: get_running_info()
            SimulationTask->>API: GET tidy3d/tasks/{id}/progress
        end
    end

    User->>webapi: download(task_id)
    webapi->>TaskFactory: get(task_id)
    TaskFactory-->>webapi: WebTask instance
    
    alt BatchTask
        webapi->>BatchTask: get_data_hdf5(CM_DATA_HDF5_GZ)
        BatchTask->>API: Download from S3
    else SimulationTask
        webapi->>SimulationTask: get_data_hdf5(SIMULATION_DATA_HDF5_GZ)
        SimulationTask->>API: Download from S3
    end

Loading

[greptile-apps]

Additional Comments (3)

1. tidy3d/web/core/task_core.py, line 896-940 (link)

   logic: pay_type and priority parameters accepted but not sent to API - raises NotImplementedError instead. Either send them to the API endpoint or remove from signature to avoid confusion.

2. tidy3d/web/tests/conftest.py, line 12-13 (link)

   logic: Duplicate TaskFactory.reset() call - one is sufficient

3. tidy3d/web/core/task_core.py, line 978-991 (link)

   logic: Redundant double-checking: If kind not in registry, checks is_batch(), registers, then calls .get() which makes another HTTP call. For simulation tasks, calls SimulationTask.get() twice (lines 983 and 988). Cache the task after first retrieval instead of re-fetching.

_{No files reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}