Set default path names #3079

momchil-flex · 2025-12-11T14:58:13Z

Replaces #2993

@yaugenst @marcorudolphflex @daquintero probably too ambitious to aim for 2.10. :D

Greptile Overview

Greptile Summary

Refactors path handling to use task-type-specific defaults when path is omitted, replacing hardcoded logic with centralized DEFAULT_DATA_FILENAME configuration. Key changes include making the path parameter optional in run(), download(), and load() functions, with defaults of simulation_data.hdf5 for FDTD/MODE/EME/HEAT tasks and cm_data.hdf5 for component modeler tasks.

Major improvements:

Task-type-specific default filenames via DEFAULT_DATA_FILENAME dictionary mapping
User-provided paths are now always respected (no silent overwriting)
Added _get_default_path() helper function to centralize path resolution logic
Updated Job class methods and autograd integration to support optional paths
Documentation updated to reflect new behavior

Issues found:

Redundant API call in download() function defeats the optimization purpose - _get_default_path() fetches the task but download() fetches it again immediately after

Confidence Score: 3/5

Safe to merge with one critical performance issue that should be fixed
The refactoring is well-structured with proper abstraction and comprehensive updates across all affected modules. However, the redundant TaskFactory.get() call in download() defeats the stated 50-67% API latency optimization goal. The logic is sound and backwards-compatible, but this performance regression needs addressing.
Pay close attention to tidy3d/web/api/webapi.py - the redundant API call in the download() function needs to be fixed to achieve the promised performance improvements

Important Files Changed

File Analysis

Filename	Score	Overview
tidy3d/web/api/webapi.py	3/5	Added `DEFAULT_DATA_FILENAME` mapping and helper functions; refactored `download()`, `load()`, and `run()` to use optional path with task-type defaults. Contains redundant API call in `download()` function.
tidy3d/web/api/container.py	4/5	Updated `Job` class methods (`run()`, `download()`, `load()`) to support optional path parameter; added `_task_type_hint()` and `_resolve_output_path()` helper methods. Clean implementation with proper caching support.

Sequence Diagram

sequenceDiagram
    participant User
    participant API as web.download()/load()
    participant Helper as _get_default_path()
    participant TaskFactory
    participant Server as get_info()

    User->>API: download(task_id, path=None)
    API->>Helper: _get_default_path(task_id, None)
    Helper->>TaskFactory: get(task_id)
    TaskFactory-->>Helper: task object
    Helper->>Helper: Check if BatchTask
    alt is BatchTask
        Helper->>Helper: task_type = "RF"
    else not BatchTask
        Helper->>Server: get_info(task_id)
        Server-->>Helper: task_info
        Helper->>Helper: Extract taskType
    end
    Helper->>Helper: default_data_filename(task_type)
    Helper-->>API: (Path, is_batch, task_type)
    API->>TaskFactory: get(task_id) [REDUNDANT]
    TaskFactory-->>API: task object
    API->>API: Download data to resolved_path

greptile-apps

_{5 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2025-12-11T15:02:00Z

tidy3d/web/api/webapi.py

+    resolved_path, is_batch, task_type = _get_default_path(task_id, path)
    task = TaskFactory.get(task_id, verbose=False)


logic: redundant TaskFactory.get() call defeats optimization purpose - _get_default_path() already fetches task at line 107, should return and reuse it

Suggested change

resolved_path, is_batch, task_type = _get_default_path(task_id, path)

task = TaskFactory.get(task_id, verbose=False)

resolved_path, is_batch, task_type, task = _get_default_path(task_id, path)

Prompt To Fix With AI

This is a comment left during a code review. Path: tidy3d/web/api/webapi.py Line: 1045:1046 Comment: **logic:** redundant `TaskFactory.get()` call defeats optimization purpose - `_get_default_path()` already fetches task at line 107, should return and reuse it ```suggestion resolved_path, is_batch, task_type, task = _get_default_path(task_id, path) ``` How can I resolve this? If you propose a fix, please make it concise.

Copilot

Pull request overview

This pull request introduces default path names for different task types, eliminating the need for users to specify the path parameter when downloading simulation results. The default filename is automatically determined based on the task type (e.g., simulation_data.hdf5 for FDTD simulations, cm_data.hdf5 for component modeler tasks).

Key changes:

Added task-type-specific default filenames that are automatically selected when path=None
Modified API signatures to make path parameter optional in run(), download(), load(), and related methods
Introduced helper functions to determine the appropriate default filename based on task type

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
tidy3d/web/api/webapi.py	Added `DEFAULT_DATA_FILENAME` mapping and `default_data_filename()` function; introduced `_get_default_path()` helper; updated `run()`, `download()`, `load()`, and `restore_simulation_if_cached()` to support optional path parameter with type-specific defaults
tidy3d/web/api/container.py	Added `_task_type_hint()` and `_resolve_output_path()` helper methods to Job class; updated `run()`, `download()`, and `load()` methods to handle optional path parameter; added `_cached_task_type` attribute for tracking task types from cache
tidy3d/web/api/autograd/engine.py	Updated `_run_tidy3d()` to use default path resolution when path is not provided
tidy3d/web/api/autograd/autograd.py	Updated `run()` function to resolve default paths based on simulation stub type
docs/api/microwave/component_modeler.rst	Added documentation noting that component modeler results default to `cm_data.hdf5` when path is omitted

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-11T15:04:45Z

tidy3d/web/api/webapi.py

+    resolved_path, is_batch, task_type = _get_default_path(task_id, path)
    task = TaskFactory.get(task_id, verbose=False)


The task is fetched twice: once inside _get_default_path at line 107 and again here at line 1046. This is inefficient as it makes two network calls to retrieve the same task information. Consider modifying _get_default_path to return the task object as well, or refactor to avoid the duplicate fetch.

Copilot · 2025-12-11T15:04:46Z

tidy3d/web/api/webapi.py

+
+    if provided_path is not None:
+        path = Path(provided_path)
+        return path, is_batch, None


When provided_path is not None, the function returns task_type=None (line 115). However, this means callers like download() will need to fetch the task type again (lines 1057-1059). Consider always fetching the task type regardless of whether a path is provided, to avoid the extra get_info call in the download function.

Suggested change

return path, is_batch, None

if is_batch:

task_type = "RF"

else:

task_info = get_info(task_id, verbose=False)

task_type = getattr(task_info, "taskType", None)

return path, is_batch, task_type

Copilot · 2025-12-11T15:04:46Z

tidy3d/web/api/container.py

+
+        try:
+            return Tidy3dStub(simulation=self.simulation).get_type()
+        except Exception:


The exception handler catches all exceptions and returns None, which may hide legitimate errors like import errors or attribute errors. Consider catching only the specific exception that indicates the task type cannot be determined (e.g., TypeError, AttributeError) to avoid masking unexpected errors.

Suggested change

except Exception:

except (AttributeError, TypeError):

Copilot · 2025-12-11T15:04:46Z

tidy3d/web/api/webapi.py

+        is_batch = False
+        task_type = None



Variable is_batch is not used.

Suggested change

is_batch = False

task_type = None

Copilot · 2025-12-11T15:04:47Z

tidy3d/web/api/webapi.py

+        task_type = None



Variable task_type is not used.

Suggested change

task_type = None

Set default path names

70595b4

Copilot AI review requested due to automatic review settings December 11, 2025 14:58

Copilot started reviewing on behalf of momchil-flex December 11, 2025 14:58 View session

greptile-apps bot reviewed Dec 11, 2025

View reviewed changes

Copilot AI reviewed Dec 11, 2025

View reviewed changes

momchil-flex mentioned this pull request Dec 12, 2025

Refactor default path handling to use task-type configuration with optional path parameter and optimized API calls #2993

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Set default path names #3079

Set default path names #3079

Uh oh!

momchil-flex commented Dec 11, 2025 •

edited by greptile-apps bot

Loading

Uh oh!

greptile-apps bot left a comment

Uh oh!

greptile-apps bot Dec 11, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 11, 2025

Uh oh!

Copilot AI Dec 11, 2025

Uh oh!

Copilot AI Dec 11, 2025

Uh oh!

Copilot AI Dec 11, 2025

Uh oh!

Copilot AI Dec 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		resolved_path, is_batch, task_type = _get_default_path(task_id, path)
		task = TaskFactory.get(task_id, verbose=False)

	resolved_path, is_batch, task_type = _get_default_path(task_id, path)
	task = TaskFactory.get(task_id, verbose=False)
	resolved_path, is_batch, task_type, task = _get_default_path(task_id, path)

-        return path, is_batch, None
+        if is_batch:
+            task_type = "RF"
+        else:
+            task_info = get_info(task_id, verbose=False)
+            task_type = getattr(task_info, "taskType", None)
+        return path, is_batch, task_type

Set default path names #3079

Are you sure you want to change the base?

Set default path names #3079

Uh oh!

Conversation

momchil-flex commented Dec 11, 2025 • edited by greptile-apps bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Overview

Greptile Summary

Confidence Score: 3/5

Important Files Changed

Sequence Diagram

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

momchil-flex commented Dec 11, 2025 •

edited by greptile-apps bot

Loading