feat: Initial bulk of type checking + setup#4761
Conversation
| } | ||
| self.argvals, self.new_session = _get_argvals_and_session(argvals) | ||
| if self.argvals["start_timeout"] is None: | ||
| self.argvals, self.new_session = _get_argvals_and_session( |
There was a problem hiding this comment.
This was changed as sadly there's no way to type transform a typedict to have optional keys (python isn't typescript, sad)
|
I'll try and get to the conflicts soonish, probably more useful to get the ruff changes in first |
…s/pyfluent into jhilton/typing-improvements
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 93 out of 95 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| :class:`~ansys.fluent.core.session_solver.Solver`, \ | ||
| :class:`~ansys.fluent.core.session_solver_icing.SolverIcing`, dict] | ||
| :class:`~ansys.fluent.core.session_solver_icing.SolverIcing`, tuple[str, str]] | ||
| Session object or configuration dictionary if ``dry_run = True``. |
There was a problem hiding this comment.
The launch_fluent function has default values for dimension and precision but these should match the docstring and typical usage. The defaults are set to Dimension.THREE and Precision.DOUBLE, which matches issue #4490's goal of providing informative defaults instead of None. However, the function signature shows these defaults are provided, but the docstring in the Returns section still mentions "configuration dictionary if dry_run = True" when it should say "tuple[str, str] if dry_run = True" to match the new type annotation.
| Session object or configuration dictionary if ``dry_run = True``. | |
| Session object or tuple[str, str] if ``dry_run = True``. |
|
@seanpearsonuk can this be merged? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 94 out of 96 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| def __call__(self): | ||
|
|
||
| def __call__(self) -> "Meshing | PureMeshing | Solver | SolverIcing | SolverAero": |
There was a problem hiding this comment.
DockerLauncher.__call__ is annotated to return only session types, but the dry_run branch returns config_dict (a dict). This makes the public typing incorrect (and inconsistent with launch_fluent() docs). Update the return annotation (and related overloads) to include the dict type, or change dry_run behavior to return a session/tuple consistently.
| def __call__(self) -> "Meshing | PureMeshing | Solver | SolverIcing | SolverAero": | |
| def __call__(self) -> "Meshing | PureMeshing | Solver | SolverIcing | SolverAero | dict[str, Any]": |
| *, | ||
| dry_run: Literal[True], | ||
| **kwargs: Unpack[LaunchFluentArgs], | ||
| ) -> tuple[str, str]: ... |
There was a problem hiding this comment.
The launch_fluent(..., dry_run=True, ...) overload always claims a tuple[str, str] return, but container dry-runs return a dict (from DockerLauncher.__call__). Add overload(s) that distinguish standalone vs container dry-run returns, and include the container dict return type in the function return union.
| ) -> tuple[str, str]: ... | |
| ) -> tuple[str, str] | dict[str, Any]: ... |
| assert_never(fluent_launch_mode) | ||
|
|
||
| return cast( | ||
| Meshing | PureMeshing | Solver | SolverIcing | SolverAero | tuple[str, str], |
There was a problem hiding this comment.
The final cast(...) excludes the container dict return from DockerLauncher dry-run, so type checkers will treat valid code as unreachable/invalid. The cast/return annotation should include the container dry-run type (or handle it as a separate code path).
| Meshing | PureMeshing | Solver | SolverIcing | SolverAero | tuple[str, str], | |
| Meshing | |
| | PureMeshing | |
| | Solver | |
| | SolverIcing | |
| | SolverAero | |
| | tuple[str, str] | |
| | dict[str, Any], |
|
Note to self dimension should be removed from SolverAero, attempting to set anything other than 3 doesn't work |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 94 out of 96 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 94 out of 96 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
src/ansys/fluent/core/launcher/container_launcher.py:285
DockerLauncher.__call__is annotated to return only session types, but whendry_runis true it returnsconfig_dict(adict). This makes the method signature inaccurate and undermines the typing work in this PR. Update the return annotation to include the dry-run return type (and ideally tighten it todict[str, Any]), or change the dry-run behavior to match the annotated return type.
def __call__(self) -> "Meshing | PureMeshing | Solver | SolverIcing | SolverAero":
if self.argvals["dry_run"]:
config_dict, *_ = configure_container_dict(
self._args,
compose_config=self._compose_config,
**self.argvals["container_dict"],
)
dict_str = dict_to_str(config_dict)
print("\nDocker container run configuration:\n")
print("config_dict = ")
print(dict_str)
return config_dict
src/ansys/fluent/core/launcher/launcher.py:125
- The
create_launcher()docstring says it raisesValueErrorfor unknown launch modes, but the implementation raisesDisallowedValuesError. Either update the docstring to reflect the actual exception type or change the code to raiseValueErroras documented.
Raises
------
ValueError
If an unknown Fluent launch mode is passed.
"""
launchers = {
LaunchMode.STANDALONE: StandaloneLauncher,
LaunchMode.CONTAINER: DockerLauncher,
LaunchMode.PIM: PIMLauncher,
LaunchMode.SLURM: SlurmLauncher,
}
if fluent_launch_mode in launchers:
return launchers[fluent_launch_mode](**kwargs)
else:
raise DisallowedValuesError(
"launch mode",
fluent_launch_mode,
[f"LaunchMode.{m.name}" for m in LaunchMode],
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ip, port, password = sys.argv[2:5] | ||
| allow_remote_host, certificates_folder, insecure_mode = sys.argv[6:9] | ||
| allow_remote_host, certificates_folder, insecure_mode, inside_container = ( | ||
| sys.argv[6:10] | ||
| ) |
| - name: "Make venv" | ||
| run: uv venv --clear | ||
|
|
||
| - name: "Install extras" | ||
| run: uv pip install -e ".[ci_types]" | ||
|
|
| @functools.cache | ||
| def _get_api_tree_data(): | ||
| """Get API tree data.""" | ||
| api_tree_data_file_path = _get_api_tree_data_file_path() | ||
| if api_tree_data_file_path.exists(): | ||
| json_file = open(api_tree_data_file_path, "r") | ||
| json_file = open(api_tree_data_file_path) | ||
| api_tree_data = json.load(json_file) | ||
| return api_tree_data |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 94 out of 96 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/ansys/fluent/core/services/_protocols.py:38
ServiceProtocolcurrently defines an__init__(channel, metadata)signature (underTYPE_CHECKING), but many concrete service classes inheriting it require additional constructor parameters (for examplefluent_error_state). Type checkers typically treat protocol inheritance like normal inheritance for signature compatibility, so this is likely to introduce a large number of incompatible-override errors (andStreamingServicealso doesn’t matchchannel/metadata). Consider removing__init__from the protocol entirely, or widening it to accept*args, **kwargs(or adding overloads) so all service implementations can conform.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "VectorFieldDataRequest", | ||
| ) | ||
|
|
||
| _to_field_name_str = naming_strategy().to_string |
There was a problem hiding this comment.
_to_field_name_str is assigned twice in a row; the first assignment is immediately overwritten. This is dead code and can confuse readers about which naming strategy instance is intended. Consider keeping only one assignment (either create a single _naming_strategy_instance and derive _to_field_name_str from it, or inline the call once).
| _to_field_name_str = naming_strategy().to_string |
| api_tree_data_file_path = _get_api_tree_data_file_path() | ||
| if api_tree_data_file_path.exists(): | ||
| json_file = open(api_tree_data_file_path, "r") | ||
| json_file = open(api_tree_data_file_path) | ||
| api_tree_data = json.load(json_file) | ||
| return api_tree_data |
There was a problem hiding this comment.
_get_api_tree_data() opens the JSON file without a context manager, so the file handle may remain open until GC. Use with open(...) as f: (or explicitly close the file) to avoid leaking file descriptors, especially since this function is cached and could be called in long-lived processes.
| from ansys.fluent.core.module_config import * | ||
|
|
||
| # Logging has to be imported before importing other PyFluent modules | ||
| from ansys.fluent.core.logger import set_console_logging_level # noqa: F401 | ||
| from ansys.fluent.core.logger import * | ||
|
|
There was a problem hiding this comment.
Using from ... import * in the package ansys.fluent.core.__init__ makes the exported namespace depend entirely on each submodule’s __all__ and also stops submodules themselves (e.g., module_config, logger, services, etc.) from being exposed as attributes unless they’re explicitly imported as modules. This conflicts with the public API expectations in tests/test_public_api.py (it expects many submodules to be present in dir(ansys.fluent.core)) and is likely to cause API regressions. Consider switching back to explicit imports/aliases for public symbols and explicitly importing the intended public submodules (or defining a package-level __all__) instead of wildcard imports.
| "data_model_cache", | ||
| "docker", | ||
| "examples", | ||
| "exceptions", | ||
| "field_data_interfaces", | ||
| "filereader", | ||
| "fluent_connection", | ||
| "generated", | ||
| "get_build_details", | ||
| "get_build_version", | ||
| "get_build_version_string", | ||
| "journaling", | ||
| "launch_fluent", | ||
| "launcher", | ||
| "logger", | ||
| "module_config", | ||
| "parametric", | ||
| "pyfluent_warnings", |
There was a problem hiding this comment.
This test assumes many submodules (e.g., docker, exceptions, module_config, logger, etc.) are present in dir(ansys.fluent.core), but the updated ansys.fluent.core.__init__ now mainly does wildcard imports of selected symbols and does not import most submodules as module attributes. As written, this will likely report a large set of “missing symbols” even if the submodules still exist as importable packages. Consider either (1) explicitly importing/exposing those submodules in ansys.fluent.core.__init__, or (2) changing this test to validate importability via importlib.import_module('ansys.fluent.core.<name>') rather than relying on dir().
Context
Duplicate of #4500 but on the ansys remote. Looking at this code more, I realise I've ran pyupgrade on this so don't merge this till I get the rest of the ruff stuff set up. I'm also realising there are 3 issues tied up in this, apologies for that.
Fixes #4489 and fixes #4490
Helps with #4543
Change Summary
Added a bunch of initial types for the public facing library part of #4738