refactor: couple of issues in launch fluent#4990
Conversation
….com/ansys/pyfluent into refactor/couple_issues_launch_fluent
Up to standards ✅🟢 Issues
|
….com/ansys/pyfluent into refactor/couple_issues_launch_fluent
|
@mayankansys Is the following happening in the lightweight-mode? bg session: read case + settings -> read journals -> make this as the main session -> apply changes that user made in the fg session The journals should be read in the background session which reads the full mesh before it. Additionally, I think the journals should be read in the foreground session also, because use will expect the journal is already read when they start making change in the activated foreground session. But I'm not sure if the journal has, e.g., a solve call, whether that will be a no-op when only settings are read. |
Thanks @mkundu1 for the question and to make sure I understand before I rework on this: The PR fixes the ordering but you're pointing at a separate problem which was, In lightweight mode the journals currently run on the foreground settings whihc doesn't have mesh. If the journal has solver call then I expect a solve in settings only session to either error or no-op which is more evidence the journals belong on the bg session. Kindly let me know what's you take on it. One issue while implementation can be the bg session is inside Before I move forwards kindly let me know:
|
…ournal_file condition
….com/ansys/pyfluent into refactor/couple_issues_launch_fluent
|
@mayankansys For the case/data+journal combination we can pass the case file name via the You can start Fluent with |
….com/ansys/pyfluent into refactor/couple_issues_launch_fluent
Was this considered? My primary intention is to avoid any client-side duplication of logic for the read-sequence. We can use the command-line flags all the time even if only case or only journal name is provided. We can get rid of all the defered_journal_read logic in the current PR. |
|
|||||||||||||||||||||||||||||||||||||||
….com/ansys/pyfluent into refactor/couple_issues_launch_fluent
| "journal_file_names" | ||
| ): | ||
| warn(LIGHTWEIGHT_MODE_IGNORED_WITH_JOURNAL, UserWarning) | ||
| self.argvals["lightweight_mode"] = False |
There was a problem hiding this comment.
Similar to the earlier conversations on this method (see below), this continues a pattern I've flagged before: accumulation of low-level, untested branching logic. I understand this follows the existing style of the method, but each addition makes the eventual refactor larger and the method harder to review as a whole.
I'd like to see argument-validation branches like this (and the timeout-string construction from before) extracted into small, testable methods rather than added inline. Given this has come up multiple times now, including in internal meetings, please treat this as a standing convention going forward, not a case-by-case judgment call.
Final changes to the same method in same PR:
seanpearsonuk
left a comment
There was a problem hiding this comment.
As per my comment.
Context
Case/case-data files should be handled before journal files, and in lightweight mode journal reading should finish before sync starts.
Change Summary
This PR fixes the launch order in launch_fluent(). It makes sure case/case-data loading happens before journal processing, and in lightweight mode it delays sync until journal execution is complete.
Impact
This affects Fluent launch behavior when using case files, case-data files, journal files, and lightweight mode. Tests were added to cover these cases.