Skip to content

FXC-4925: Remove BatchDetail and BatchTask in webapi #3183

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
momchil-flex wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

FXC-4925: Remove BatchDetail and BatchTask in webapi #3183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6480cf9
Taking BatchDetail taskType from statistics api
momchil-flex Jan 12, 2026
1df04a7
Remove BatchDetail in favor of TaskInfo
momchil-flex Jan 16, 2026
c3b1eb4
Deprecate BatchTask
momchil-flex Jan 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 10 additions & 13 deletions tests/test_web/test_webapi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,14 @@ def mock_upload_file(*args, **kwargs):
def mock_get_info(monkeypatch, set_api_key):
"""Mocks webapi.get_info."""

# Mock the batch API check (returns 404 to indicate it's not a batch task)
responses.add(
responses.GET,
f"{Env.current.web_api_endpoint}/rf/task/{TASK_ID}/statistics",
json={"error": "Not found"},
status=404,
)

responses.add(
responses.GET,
f"{Env.current.web_api_endpoint}/tidy3d/tasks/{TASK_ID}/detail",
Expand All @@ -225,6 +233,8 @@ def mock_get_info(monkeypatch, set_api_key):
"metadataStatus": "processed",
"status": "success",
"s3Storage": 1.0,
"groupId": "group123",
"version": "v1",
}
},
status=200,
Expand Down Expand Up @@ -437,19 +447,6 @@ def mock_download(*args, **kwargs):

@responses.activate
def test_delete(set_api_key, mock_get_info):
responses.add(
responses.GET,
f"{Env.current.web_api_endpoint}/tidy3d/tasks/{TASK_ID}",
json={
"data": {
"taskId": TASK_ID,
"groupId": "group123",
"version": "v1",
"createdAt": CREATED_AT,
}
},
status=200,
)

responses.add(
responses.DELETE,
Expand Down
54 changes: 25 additions & 29 deletions tidy3d/web/api/webapi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,7 @@
TaskId,
)
from tidy3d.web.core.task_core import (
BatchDetail,
BatchTask,
BatchTask, # noqa: F401 - Deprecated alias, kept for backward compatibility
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The BatchTask alias is deprecated and imported with noqa: F401 to suppress unused import warnings. However, this import should be explicitly documented in a docstring or comment explaining the deprecation timeline and migration path for users. Consider adding a deprecation warning when BatchTask is used, or at minimum document when this alias will be removed.

Suggested change
BatchTask, # noqa: F401 - Deprecated alias, kept for backward compatibility
BatchTask, # noqa: F401 - Deprecated alias.
# NOTE:
# `BatchTask` is imported here solely to preserve backward compatibility for
# external code that previously imported `BatchTask` from
# `tidy3d.web.api.webapi`. This alias is deprecated and will be removed in a
# future major release. Users should instead import `BatchTask` directly from
# `tidy3d.web.core.task_core`, or follow the migration guidance in the
# tidy3d web API documentation to use the newer task abstractions.

Copilot uses AI. Check for mistakes.
Folder,
SimulationTask,
TaskFactory,
Expand Down Expand Up @@ -116,8 +115,8 @@ def _batch_detail_error(resource_id: str) -> Optional[WebError]:

# TODO: test properly
try:
batch = BatchTask.get(resource_id)
batch_detail = batch.detail()
task = SimulationTask.get(resource_id)
batch_detail = task.detail()
status = batch_detail.status.lower()
except Exception as e:
log.error(f"Could not retrieve batch details for '{resource_id}': {e}")
Expand Down Expand Up @@ -619,7 +618,7 @@ def get_reduced_simulation(


@wait_for_connection
def get_info(task_id: TaskId, verbose: bool = True) -> TaskInfo | BatchDetail:
def get_info(task_id: TaskId, verbose: bool = True) -> TaskInfo:
"""Return information about a simulation task or a modeler batch.

This function fetches details for a given task ID, automatically
Expand All @@ -635,9 +634,8 @@ def get_info(task_id: TaskId, verbose: bool = True) -> TaskInfo | BatchDetail:

Returns
-------
TaskInfo | BatchDetail
A ``TaskInfo`` object for a standard simulation task, or a
``BatchDetail`` object for a modeler batch.
TaskInfo
An object containing information about the task or batch.

Raises
------
Expand Down Expand Up @@ -714,17 +712,17 @@ def get_run_info(task_id: TaskId) -> tuple[Optional[float], Optional[float]]:
Is ``None`` if run info not available.
"""
task = TaskFactory.get(task_id)
if isinstance(task, BatchTask):
if task._is_batch_type():
raise NotImplementedError("Operation not implemented for modeler batches.")
return task.get_running_info()


def _get_batch_detail_handle_error_status(batch: BatchTask) -> BatchDetail:
def _get_batch_detail_handle_error_status(task: SimulationTask) -> TaskInfo:
"""Get batch detail and raise error if status is in ERROR_STATES."""
detail = batch.detail()
detail = task.detail()
status = detail.status.lower()
if status in ERROR_STATES:
_batch_detail_error(batch.task_id)
_batch_detail_error(task.task_id)
return detail


Expand All @@ -737,7 +735,7 @@ def get_status(task_id: TaskId) -> str:
Unique identifier of task on server. Returned by :meth:`upload`.
"""
task = TaskFactory.get(task_id)
if isinstance(task, BatchTask):
if task._is_batch_type():
return _get_batch_detail_handle_error_status(task).status
else:
task_info = get_info(task_id)
Expand Down Expand Up @@ -788,7 +786,7 @@ def monitor(task_id: TaskId, verbose: bool = True, worker_group: Optional[str] =

# Batch/modeler monitoring path
task = TaskFactory.get(task_id)
if isinstance(task, BatchTask):
if task._is_batch_type():
return _monitor_modeler_batch(task_id, verbose=verbose)

console = get_logging_console() if verbose else None
Expand Down Expand Up @@ -985,7 +983,7 @@ def download(
"""
path = Path(path)
task = TaskFactory.get(task_id, verbose=False)
if isinstance(task, BatchTask):
if task._is_batch_type():
if path.name == "simulation_data.hdf5":
path = path.with_name("cm_data.hdf5")
task.get_data_hdf5(
Expand Down Expand Up @@ -1022,7 +1020,7 @@ def download_json(task_id: TaskId, path: PathLike = SIM_FILE_JSON, verbose: bool

"""
task = TaskFactory.get(task_id, verbose=False)
if isinstance(task, BatchTask):
if task._is_batch_type():
raise NotImplementedError("Operation not implemented for modeler batches.")
task.get_simulation_json(path, verbose=verbose)

Expand Down Expand Up @@ -1055,7 +1053,7 @@ def load_simulation(
Simulation loaded from downloaded json file.
"""
task = TaskFactory.get(task_id, verbose=False)
if isinstance(task, BatchTask):
if task._is_batch_type():
raise NotImplementedError("Operation not implemented for modeler batches.")
path = Path(path)
if path.suffix == ".json":
Expand Down Expand Up @@ -1092,7 +1090,7 @@ def download_log(
To load downloaded results into data, call :meth:`load` with option ``replace_existing=False``.
"""
task = TaskFactory.get(task_id, verbose=False)
if isinstance(task, BatchTask):
if task._is_batch_type():
raise NotImplementedError("Operation not implemented for modeler batches.")
task.get_log(path, verbose=verbose, progress_callback=progress_callback)

Expand Down Expand Up @@ -1145,12 +1143,10 @@ def load(
"""
path = Path(path)
task = TaskFactory.get(task_id) if task_id else None
# Check if task is a batch type (handle mocked objects that may not have the method)
is_batch = task is not None and getattr(task, "_is_batch_type", lambda: False)()
Comment on lines +1146 to +1147
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using getattr with a lambda default is defensive but could hide underlying issues. If 'task' is not None but doesn't have '_is_batch_type', this indicates a type inconsistency that should be addressed at the source. Consider adding a type check or assertion here to ensure 'task' is actually a SimulationTask when not None, rather than silently handling the case where the method doesn't exist.

Suggested change
# Check if task is a batch type (handle mocked objects that may not have the method)
is_batch = task is not None and getattr(task, "_is_batch_type", lambda: False)()
if task is not None and not isinstance(task, SimulationTask):
raise TypeError(
f"Expected 'SimulationTask' from TaskFactory.get for task_id={task_id!r}, "
f"got {type(task).__name__!r} instead."
)
is_batch = bool(task is not None and task._is_batch_type())

Copilot uses AI. Check for mistakes.
# For component modeler batches, default to a clearer filename if the default was used.
if (
task_id
and isinstance(task, BatchTask)
and path.name in {"simulation_data.hdf5", "simulation_data.hdf5.gz"}
):
if task_id and is_batch and path.name in {"simulation_data.hdf5", "simulation_data.hdf5.gz"}:
path = path.with_name(path.name.replace("simulation", "cm"))

if task_id is None:
Expand All @@ -1161,7 +1157,7 @@ def load(

if verbose and task_id is not None:
console = get_logging_console()
if isinstance(task, BatchTask):
if is_batch:
console.log(f"Loading component modeler data from {path}")
else:
console.log(f"Loading simulation from {path}")
Expand Down Expand Up @@ -1201,9 +1197,9 @@ def _monitor_modeler_batch(
) -> None:
"""Monitor modeler batch progress with aggregate and per-task views."""
console = get_logging_console() if verbose else None
task = BatchTask.get(task_id=task_id)
task = SimulationTask.get(task_id=task_id)
detail = _get_batch_detail_handle_error_status(task)
name = detail.name or "modeler_batch"
Copy link

@cursor cursor bot Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling len() on potentially None tasks field

Medium Severity

The tasks field in TaskInfo defaults to None (changed from [] in the removed BatchDetail class), but _monitor_modeler_batch calls len(detail.tasks) at line 1237 without checking for None first. If the batch API response isn't a dict or the transformation in detail() is bypassed, tasks would be None, causing a TypeError: object of type 'NoneType' has no len().

Additional Locations (1)

Fix in Cursor Fix in Web

name = detail.taskName or "modeler_batch"
group_id = detail.groupId
status = detail.status.lower()

Expand Down Expand Up @@ -1346,7 +1342,7 @@ def download_simulation(

"""
task = TaskFactory.get(task_id, verbose=False)
if isinstance(task, BatchTask):
if task._is_batch_type():
raise NotImplementedError("Operation not implemented for modeler batches.")
info = get_info(task_id, verbose=False)
remote_sim_file = SIM_FILE_HDF5_GZ
Expand Down Expand Up @@ -1451,7 +1447,7 @@ def estimate_cost(

task = TaskFactory.get(task_id, verbose=False)
detail = task.detail()
if isinstance(task, BatchTask):
if task._is_batch_type():
check_task_type = "FDTD" if detail.taskType == "MODAL_CM" else "RF_FDTD"
task.check(solver_version=solver_version, check_task_type=check_task_type)
detail = task.detail()
Expand Down Expand Up @@ -1573,7 +1569,7 @@ def real_cost(task_id: str, verbose: bool = True) -> float | None:
else:
if verbose:
console.log(f"Billed flex credit cost: {flex_unit:1.3f}.")
if flex_unit != ori_flex_unit and "FDTD" in task_info.taskType:
if flex_unit != ori_flex_unit and task_info.taskType and "FDTD" in task_info.taskType:
console.log(
"Note: the task cost pro-rated due to early shutoff was below the minimum "
"threshold, due to fast shutoff. Decreasing the simulation 'run_time' should "
Expand Down
Loading
Loading