-
Notifications
You must be signed in to change notification settings - Fork 57
chore: Update launcher tests for better coverage #4629
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
Conversation
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 blocks the use of PureMeshing.from_connection() to prevent confusion, since PureMeshing is a PyFluent-specific concept with no way to distinguish it from a running Meshing session. The change raises a RuntimeError when attempting to create a PureMeshing session via from_connection(), while other factory methods remain available.
Key Changes:
- Added runtime check in
from_connection()to preventPureMeshingconnections - Extended test coverage to verify new launcher APIs for
SolverAeroandMeshing - Added test case confirming
PureMeshing.from_connection()raisesRuntimeError
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/ansys/fluent/core/session_utilities.py | Added validation to raise RuntimeError for PureMeshing.from_connection() |
| tests/test_session.py | Extended tests to cover SolverAero and Meshing launcher APIs, plus PureMeshing error case |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
A question related to this PR is, whether to get rid of 'PureMeshing' completely from these factory patterns? The reason why I did not do it is that it makes sense for all the other methods while only creates an issue for "from_connection". Another question is if my thinking makes sense, whether to raise an AttributeError and remove it from the attribute of PureMeshing? Finally, to avoid all these confusions, is it better to completely remove 'PureMeshing' from here like earlier discussed. Please provide your thoughts on this. cc. @seanpearsonuk, @mkundu1 |
|
It is ok to create a pure session from an "impure" session. But the reverse is not true because if it really is originally pure then switching to solver won't work. No such limitations exist the other way. I believe that that is what I said in the meeting but feel free to share your views. @prmukherj @mkundu1 @hpohekar @mayankansys @raph-luc @Gobot1234 |
Ok, then we will remove it completely like discussed, because there is no way to check the session type for Pure Meshing session from the server side. Thank you |
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 2 out of 2 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.
Before we remove it completely, I’d like to restate why I think retaining the pure meshing interface is still valuable. The motivation behind “Pure Meshing” wasn’t meant to be a PyFluent-only abstraction. It supports a modular workflow where users can deliberately choose to isolate meshing from solving. In particular:
So while we can’t currently detect a “pure” session from the server, users can still intentionally choose to work in a segregated way. Removing this option takes away a valid pattern that is relevant in automated and cloud-centric deployments. |
@prmukherj @mkundu1 @hpohekar @mayankansys @raph-luc @Gobot1234 |
I agree completely with this @seanpearsonuk. That's why it's better just to leave it as it is and might be document this limitation? |
|
Can I suggest the following in the body of the PureMeshing class if you are going to retain it but keep it as an error if TYPE_CHECKING:
@typing_extensions.deprecated("This will always raise due a fluent limitation")
def from_connection(cls, *args: Any, **kwargs: Any) -> typing_extensions.Never: ... |
In fact, connecting a pure client to a mixed-mode server is fine. This is just interface segregation. |
|
Next steps?
|
Will do it after this release PR is merged. Thank you. |
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 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Done. |
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 4 out of 4 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.
|
Thank you @seanpearsonuk for the suggestions. Applied them. |
@seanpearsonuk I agree. The container specific requirements specified here should consider for 26R2 planning as well. |
Context
Extensive test coverage for the launcher APIs from session classes is introduced.
One issue is when a 'PureMeshing' session is launched from PyFluent and 'Meshing' session is connected to it using the launcher api, it might be confusing for users, as now no mechanism exists in server to inform client that it has been launched in 'PureMeshing' mode. Hence, this limitation is to be documented.
Change Summary
Tests were added for extensive coverage of launcher APIs and the limitation is documented.
Impact
No direct impact, only a limitation as mentioned above to be documented.
There are four possible connection patterns:
Meshing → Meshing | ✔ Safe | Full capabilities preserved
PureMeshing → PureMeshing | ✔ Safe | Capabilities match exactly
Meshing → PureMeshing | ✔ Allowed | API connects to a subset of available capabilities
PureMeshing → Meshing | ⚠ Not recommended | Meshing API expects solver features that a pure-meshing session cannot provide
The fourth scenario is problematic because Meshing may assume capabilities (e.g., solver mode switching) that are not enabled in a pure-meshing session, leading to inconsistent or undefined behavior.
In later Fluent release we might consider updating the app-utilities, as suggested by @mkundu1
"An implementation can be done by adding a new method set_pure_meshing_app_mode in the app_utilities service. The new method will be called to set a flag in Fluent when the Fluent session is launched or connected from a PureMeshing PyFluent session. That flag will be checked in get_app_modemethod of app_utilities. We can also block switch-to-solver operation within Fluent when that flag is set."