-
Notifications
You must be signed in to change notification settings - Fork 59
feat: Typing improvements #4761
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.
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
base: main
Are you sure you want to change the base?
Conversation
| UnexpectedKeywordArgument | ||
| If an unexpected keyword argument is provided. | ||
| Notes | ||
| ----- | ||
| In job scheduler environments (e.g., SLURM, LSF, PBS), resources and compute nodes are allocated, | ||
| and core counts are queried from these environments before being passed to Fluent. | ||
| """ | ||
| insecure_mode_env = os.getenv("PYFLUENT_CONTAINER_INSECURE_MODE") == "1" | ||
| if certificates_folder is None and not insecure_mode and not insecure_mode_env: | ||
| raise ValueError( | ||
| "To launch Fluent in secure gRPC mode, set `certificates_folder`." | ||
| ) | ||
| if certificates_folder is not None and insecure_mode: | ||
| raise ValueError( | ||
| "`certificates_folder` and `insecure_mode` cannot be set at the same time." | ||
| ) | ||
|
|
||
| locals_ = locals().copy() | ||
| argvals = { | ||
| arg: locals_.get(arg) | ||
| for arg in inspect.getargvalues(inspect.currentframe()).args | ||
| } | ||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements typing improvements for the PyFluent codebase by modernizing type annotations to use Python 3.9+ syntax. The changes upgrade type hints from typing module classes to built-in generic types, remove unnecessary imports, and add comprehensive type stubs for public-facing APIs.
Changes:
- Replaced
typing.Dict,typing.List,typing.Tuple,typing.Callable, andtyping.Iterablewith their built-in equivalents (dict,list,tuple, andcollections.abccounterparts) - Removed obsolete imports such as
from builtins import rangeand unusedtyping.Dict/Listimports - Added type annotations and overloads to session utility methods for better type inference
- Updated file opening operations to remove unnecessary mode arguments
- Added
__all__exports for better API documentation
Reviewed changes
Copilot reviewed 87 out of 88 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_scheme_eval_231.py | Updated type hints from Dict to dict |
| tests/test_scheduler.py | Removed unused builtins.range import |
| tests/test_new_meshing_workflow.py | Moved Iterable import from typing to collections.abc |
| tests/test_meshingmode/test_meshing_launch.py | Simplified super() calls by removing redundant class/self arguments |
| tests/test_builtin_settings.py | Removed unused imports |
| tests/fluent_fixtures.py | Moved Callable from typing to collections.abc |
| src/ansys/fluent/core/workflow.py | Updated type hints to use built-in generics and collections.abc |
| src/ansys/fluent/core/utils/setup_for_fluent.py | Added __all__ export |
| src/ansys/fluent/core/utils/fluent_version.py | Added __all__ export |
| src/ansys/fluent/core/utils/fldoc.py | Removed Python 3.9-specific version check code |
| src/ansys/fluent/core/utils/execution.py | Moved Callable to collections.abc |
| src/ansys/fluent/core/utils/deprecate.py | Moved Callable to collections.abc |
| src/ansys/fluent/core/utils/init.py | Added __all__ exports and removed unused import |
| src/ansys/fluent/core/ui/standalone_web_ui.py | Updated type hints from Dict/List to dict/list |
| src/ansys/fluent/core/system_coupling.py | Updated type hints and removed mode argument from file open |
| src/ansys/fluent/core/streaming_services/*.py | Moved Callable to collections.abc across multiple files |
| src/ansys/fluent/core/solver/*.py | Updated type hints to built-in generics and added type checking guards |
| src/ansys/fluent/core/session*.py | Added comprehensive type stubs and overloads for session classes |
| src/ansys/fluent/core/services/*.py | Added ServiceProtocol base class and updated type hints |
| src/ansys/fluent/core/launcher/*.py | Added TypedDict definitions and updated launcher signatures |
| src/ansys/fluent/core/*.py | Various typing improvements across core modules |
| pyproject.toml | Added typing-extensions dependency and basedpyright configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| IntListType = List[int] | ||
| StringListType = List[str] | ||
| BoolListType = List[bool] | ||
| RealType = NewType("real", float | str) # constant or expression |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment "# constant or expression" should be updated to reflect that this is a NewType definition. The Union type should also be placed in quotes for forward compatibility: NewType("real", "float | str").
| RealType = NewType("real", float | str) # constant or expression | |
| RealType = NewType("real", "float | str") # NewType for real constant or expression |
|
Still need to add a baseline file for this and turn on basedpyright |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change to star imports makes everything exported as public to to typechecker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gobot1234 We can't lose control over the public API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry if it wasn't clear you don't the modules define a dunder all and that's how public symbols are decided there is no regression in pyfluent's list of publicly available symbols
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Gobot1234
Can you kindly explain the last comment in more detail. I was not able to understand it properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gobot1234 OK, I didn't see that you had added those, and I confirm that I see those now. I can ask @mayankansys to add a test for dir() at the top-level where the expected result is based on current behaviour. Or does such a test exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Gobot1234 Can you kindly explain the last comment in more detail. I was not able to understand it properly.
A little on `__all__` if you weren't sure about that
Python has a special symbol __all__ which is a sequence of str at the module level which defines the symbols that get imported when you do from ... import *. This stops things leaking into the module scope. Normally it just gets all attributes that don't have a leading underscore in the module
This is the result of adding the all to the submodules (it should look basically no different), I have however adjusted some of the imports in pyfluent that aren't meant to be there.
>>> dir()
['PS1', 'REPLHooks', '__annotations__', '__builtins__', '__doc__', '__loader__', '__name__', '__package__', '__spec__', 'get_last_command', 'is_wsl', 'original_ps1', 'platform', 'sys']
>>> import ansys.fluent.core as pyfluent
>>> dir(pyfluent)
['AboutToInitializeSolutionEventInfo', 'AboutToLoadCaseEventInfo', 'AboutToLoadDataEventInfo', 'BaseSession', 'BatchOps', 'CalculationsEndedEventInfo', 'CalculationsPausedEventInfo', 'CalculationsResumedEventInfo', 'CalculationsStartedEventInfo', 'CaseLoadedEventInfo', 'DataLoadedEventInfo', 'Dimension', 'Event', 'EventsManager', 'FatalErrorEventInfo', 'Fluent', 'FluentDevVersionWarning', 'FluentLinuxGraphicsDriver', 'FluentMode', 'FluentVersion', 'FluentWindowsGraphicsDriver', 'IterationEndedEventInfo', 'LocalParametricStudy', 'Meshing', 'MeshingEvent', 'PathlinesFieldDataRequest', 'PrePost', 'Precision', 'ProgressUpdatedEventInfo', 'PureMeshing', 'PyFluentDeprecationWarning', 'PyFluentUserWarning', 'ReportDefinitionUpdatedEventInfo', 'ReportPlotSetUpdatedEventInfo', 'ResidualPlotUpdatedEventInfo', 'ScalarFieldDataRequest', 'SettingsClearedEventInfo', 'SolutionInitializedEventInfo', 'SolutionPausedEventInfo', 'Solver', 'SolverAero', 'SolverEvent', 'SolverIcing', 'SolverTimeEstimateUpdatedEventInfo', 'SurfaceDataType', 'SurfaceFieldDataRequest', 'TimestepEndedEventInfo', 'TimestepStartedEventInfo', 'UIMode', 'VectorFieldDataRequest', '_README_FILE', '_THIS_DIRNAME', '_TYPE_CHECKING', '_VERSION_INFO', '__builtins__', '__cached__', '__doc__', '__file__', '__getattr__', '__loader__', '__name__', '__package__', '__path__', '__spec__', '__version__', '_config_by_deprecated_name', '_fldoc', '_os', '_pydoc', '_types', '_warnings', 'config', 'create_launcher', 'data_model_cache', 'docker', 'exceptions', 'field_data_interfaces', 'filereader', 'fluent_connection', 'get_build_details', 'get_build_version', 'get_build_version_string', 'get_user_data_dir', 'journaling', 'launch_fluent', 'launcher', 'load_module', 'logger', 'meshing', 'module_config', 'parametric', 'pyfluent_warnings', 'rpvars', 'scheduler', 'search', 'services', 'session', 'session_base_meshing', 'session_meshing', 'session_pure_meshing', 'session_shared', 'session_solver', 'session_solver_aero', 'session_solver_icing', 'session_utilities', 'set_console_logging_level', 'setup_for_fluent', 'solver', 'streaming_services', 'system_coupling', 'utils', 'variable_strategies', 'version_info', 'warning', 'workflow', 'workflow_new']
>>> from ansys.fluent.core import *
>>> dir()
['AboutToInitializeSolutionEventInfo', 'AboutToLoadCaseEventInfo', 'AboutToLoadDataEventInfo', 'BaseSession', 'BatchOps', 'CalculationsEndedEventInfo', 'CalculationsPausedEventInfo', 'CalculationsResumedEventInfo', 'CalculationsStartedEventInfo', 'CaseLoadedEventInfo', 'DataLoadedEventInfo', 'Dimension', 'Event', 'EventsManager', 'FatalErrorEventInfo', 'Fluent', 'FluentDevVersionWarning', 'FluentLinuxGraphicsDriver', 'FluentMode', 'FluentVersion', 'FluentWindowsGraphicsDriver', 'IterationEndedEventInfo', 'LocalParametricStudy', 'Meshing', 'MeshingEvent', 'PS1', 'PathlinesFieldDataRequest', 'PrePost', 'Precision', 'ProgressUpdatedEventInfo', 'PureMeshing', 'PyFluentDeprecationWarning', 'PyFluentUserWarning', 'REPLHooks', 'ReportDefinitionUpdatedEventInfo', 'ReportPlotSetUpdatedEventInfo', 'ResidualPlotUpdatedEventInfo', 'ScalarFieldDataRequest', 'SettingsClearedEventInfo', 'SolutionInitializedEventInfo', 'SolutionPausedEventInfo', 'Solver', 'SolverAero', 'SolverEvent', 'SolverIcing', 'SolverTimeEstimateUpdatedEventInfo', 'SurfaceDataType', 'SurfaceFieldDataRequest', 'TimestepEndedEventInfo', 'TimestepStartedEventInfo', 'UIMode', 'VectorFieldDataRequest', '__annotations__', '__builtins__', '__doc__', '__loader__', '__name__', '__package__', '__spec__', 'config', 'create_launcher', 'data_model_cache', 'docker', 'exceptions', 'field_data_interfaces', 'filereader', 'fluent_connection', 'get_build_details', 'get_build_version', 'get_build_version_string', 'get_last_command', 'get_user_data_dir', 'is_wsl', 'journaling', 'launch_fluent', 'launcher', 'load_module', 'logger', 'meshing', 'module_config', 'original_ps1', 'parametric', 'platform', 'pyfluent', 'pyfluent_warnings', 'rpvars', 'scheduler', 'search', 'services', 'session', 'session_base_meshing', 'session_meshing', 'session_pure_meshing', 'session_shared', 'session_solver', 'session_solver_aero', 'session_solver_icing', 'session_utilities', 'set_console_logging_level', 'setup_for_fluent', 'solver', 'streaming_services', 'sys', 'system_coupling', 'utils', 'variable_strategies', 'version_info', 'warning', 'workflow', 'workflow_new']There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
@Gobot1234 |
No I decided against it it's currently too hard to setup and be meaningfully useful without being able to baseline all the errors so they can be fixed gradually |
| certificates_folder, insecure_mode or insecure_mode_env | ||
| kwargs.get("certificates_folder"), | ||
| kwargs.get("insecure_mode") or insecure_mode_env, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkundu1 am I missing something here it seems like this shouldn't be causing the docker build errors that are currently in CI 🤔
(There should probably be .get("insecure_mode", False) though will add)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gobot1234 We are updating the user-provided values of certificates_folder and insecure_mode. The new values should be put in kwargs as that is passed in below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 88 out of 89 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not doc: | ||
| doc = pydoc.getdoc(object) | ||
| if doc: |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removed code handled differences in Python 3.9+ behavior. While the PR description mentions pyupgrade changes, removing version-specific logic without ensuring compatibility across supported Python versions could introduce issues. Verify that this change works correctly across all supported Python versions (the codebase appears to support 3.9+).
| kwargs_with_mode = dict(kwargs) | ||
| kwargs_with_mode["mode"] = cls._session_mode[cls.__name__] | ||
| launcher = PIMLauncher(**kwargs_with_mode) |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating a new dictionary and mutating it is less clean than using dictionary unpacking. Consider using launcher = PIMLauncher(**kwargs, mode=cls._session_mode[cls.__name__]) for better readability.
| kwargs_with_mode = dict(kwargs) | |
| kwargs_with_mode["mode"] = cls._session_mode[cls.__name__] | |
| launcher = PIMLauncher(**kwargs_with_mode) | |
| launcher = PIMLauncher(**kwargs, mode=cls._session_mode[cls.__name__]) |
| additional_arguments = kwargs.get("additional_arguments", "") | ||
| start_watchdog = kwargs.get("start_watchdog") | ||
| file_transfer_service = kwargs.get("file_transfer_service") |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These variables are extracted from kwargs but only used once within warning conditions. Consider accessing them directly in the warning checks to reduce unnecessary variable assignments.
| from typing import TypeAlias | ||
|
|
||
| PathType: TypeAlias = "os.PathLike[str] | os.PathLike[bytes] | str | bytes" | ||
| PathType: TypeAlias = "os.PathLike[str] | str" |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing bytes and os.PathLike[bytes] from PathType is a breaking change that could affect existing code passing bytes paths. This should be documented in the changelog or reconsidered if bytes paths are used in the codebase.
| PathType: TypeAlias = "os.PathLike[str] | str" | |
| PathType: TypeAlias = "os.PathLike[str] | os.PathLike[bytes] | str | bytes" |
| argval = fluent_map[json_key] | ||
| launch_args_string += v["fluent_format"].replace("{}", str(argval)) | ||
| additional_arguments = kwargs["additional_arguments"] | ||
| additional_arguments = kwargs.get("additional_arguments", "") |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using .get() with a default is safer than accessing kwargs directly. However, if additional_arguments is guaranteed to exist in kwargs based on the TypedDict definitions, direct access kwargs['additional_arguments'] would be more explicit and would fail fast if the contract is violated.
| steps: | ||
| - name: "Run PyAnsys code style checks" | ||
| uses: ansys/actions/[email protected] | ||
| - uses: ansys/actions/code-style@v10 |
Check warning
Code scanning / CodeQL
Unpinned tag for a non-immutable Action in workflow Medium
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