-
Notifications
You must be signed in to change notification settings - Fork 74
refactor: avoid deleting server-info file #5242
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?
Changes from 52 commits
fc8c72b
f94faa6
25a3143
2065946
c359acd
9aea156
481f46f
8d64547
ae8f3cc
bf31ed6
26d259f
bbcd2fd
482b20f
7c5fd0c
b74d226
5ec458e
28135e9
98945db
1bc029c
3563a14
0a2861f
16f320a
a9b23b9
3423fa1
e0c0475
765b43e
53399af
b60bdf3
b03e175
3da7114
011c0d4
d8c4cf4
2c4c1c4
8e3563e
15713e8
dd05b8e
6df1995
a176ec0
16ca1a4
3c05c73
b5a18a3
cf6eba3
d439a2f
1a75495
53d63ba
fec936a
ca79203
8389028
9c5c991
e0925c9
6d876b0
ff200a0
d50d644
fca9825
7b74ebd
df73f8d
dbf08cc
9dc1751
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Avoid deleting server-info file |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,8 @@ | |
| import platform | ||
| import tempfile | ||
| from tempfile import TemporaryDirectory | ||
| from unittest.mock import MagicMock, Mock, patch | ||
| import warnings | ||
|
|
||
| import pytest | ||
|
|
||
|
|
@@ -799,3 +801,119 @@ def test_idle_timeout(monkeypatch): | |
| StandaloneLauncher._construct_timeout_arg(200) | ||
| == ' -command="(set-session-idle-timeoutPLF+5)"' | ||
| ) | ||
|
|
||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we know the history of this class? Why are PyFluent tests checking the mechanics of Python's documented file handling functionality? We should just delete this - and anything else with a similar smell.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Noted. |
||
| def test_standalone_launcher_cleanup_on_exit_false_preserves_file(): | ||
| """Verify old cleanup_on_exit=False parameter still works (backward compatibility).""" | ||
| from pathlib import Path | ||
|
|
||
| with tempfile.TemporaryDirectory() as tmp_dir: | ||
| server_info_file = Path(tmp_dir) / "serverinfo-test.txt" | ||
| server_info_file.write_text("test content") | ||
|
|
||
| cleanup_on_exit = False | ||
| if cleanup_on_exit: | ||
| server_info_file.unlink(missing_ok=True) | ||
|
|
||
| assert ( | ||
| server_info_file.exists() | ||
| ), "File should be preserved with cleanup_on_exit=False" | ||
|
|
||
|
|
||
| def test_deprecation_cleanup_on_exit_true_converts_to_preserve_false(): | ||
| """Verify cleanup_on_exit=True converts to preserve_info_file=False (inverted logic).""" | ||
| from ansys.fluent.core.launcher.fluent_container import ( | ||
| _cleanup_on_exit_to_preserve_info_file_converter, | ||
| ) | ||
|
|
||
| # The converter function doesn't issue warnings; the decorator does | ||
| kwargs = {"cleanup_on_exit": True} | ||
| result = _cleanup_on_exit_to_preserve_info_file_converter( | ||
| kwargs, ["cleanup_on_exit"], ["preserve_info_file"] | ||
| ) | ||
|
|
||
| # Should be converted to preserve_info_file=False | ||
| assert ( | ||
| result.get("preserve_info_file") is False | ||
| ), "cleanup_on_exit=True should convert to preserve_info_file=False" | ||
| assert ( | ||
| "cleanup_on_exit" not in result | ||
| ), "cleanup_on_exit should be removed from kwargs" | ||
|
|
||
|
|
||
| def test_deprecation_cleanup_on_exit_false_converts_to_preserve_true(): | ||
| """Verify cleanup_on_exit=False converts to preserve_info_file=True (inverted logic).""" | ||
| from ansys.fluent.core.launcher.fluent_container import ( | ||
| _cleanup_on_exit_to_preserve_info_file_converter, | ||
| ) | ||
|
|
||
| # The converter function doesn't issue warnings; the decorator does | ||
| kwargs = {"cleanup_on_exit": False} | ||
| result = _cleanup_on_exit_to_preserve_info_file_converter( | ||
| kwargs, ["cleanup_on_exit"], ["preserve_info_file"] | ||
| ) | ||
|
|
||
| # Should be converted to preserve_info_file=True | ||
| assert ( | ||
| result.get("preserve_info_file") is True | ||
| ), "cleanup_on_exit=False should convert to preserve_info_file=True" | ||
| assert ( | ||
| "cleanup_on_exit" not in result | ||
| ), "cleanup_on_exit should be removed from kwargs" | ||
|
|
||
|
|
||
| def test_new_preserve_info_file_no_deprecation_warning(): | ||
| """Verify new preserve_info_file parameter works without deprecation warnings.""" | ||
| with warnings.catch_warnings(record=True) as w: | ||
| warnings.simplefilter("always") | ||
| from ansys.fluent.core.launcher.fluent_container import ( | ||
| _cleanup_on_exit_to_preserve_info_file_converter, | ||
| ) | ||
|
|
||
| # Test with preserve_info_file=True | ||
| kwargs = {"preserve_info_file": True} | ||
| result = _cleanup_on_exit_to_preserve_info_file_converter( | ||
| kwargs, ["cleanup_on_exit"], ["preserve_info_file"] | ||
| ) | ||
| deprecation_warnings = [ | ||
| warning | ||
| for warning in w | ||
| if issubclass(warning.category, PyFluentDeprecationWarning) | ||
| ] | ||
| assert len(deprecation_warnings) == 0, "Should not warn with new parameter" | ||
| assert result.get("preserve_info_file") is True | ||
|
|
||
| # Test with preserve_info_file=False | ||
| w.clear() | ||
| kwargs = {"preserve_info_file": False} | ||
| result = _cleanup_on_exit_to_preserve_info_file_converter( | ||
| kwargs, ["cleanup_on_exit"], ["preserve_info_file"] | ||
| ) | ||
| deprecation_warnings = [ | ||
| warning | ||
| for warning in w | ||
| if issubclass(warning.category, PyFluentDeprecationWarning) | ||
| ] | ||
| assert len(deprecation_warnings) == 0, "Should not warn with new parameter" | ||
| assert result.get("preserve_info_file") is False | ||
|
|
||
|
|
||
| def test_both_cleanup_on_exit_and_preserve_info_file_provided(): | ||
| """Verify providing both parameters warns and uses new one.""" | ||
| import pytest | ||
|
|
||
| with pytest.warns(PyFluentDeprecationWarning, match="Both deprecated"): | ||
| from ansys.fluent.core.launcher.fluent_container import ( | ||
| _cleanup_on_exit_to_preserve_info_file_converter, | ||
| ) | ||
|
|
||
| kwargs = {"cleanup_on_exit": True, "preserve_info_file": False} | ||
| result = _cleanup_on_exit_to_preserve_info_file_converter( | ||
| kwargs, ["cleanup_on_exit"], ["preserve_info_file"] | ||
| ) | ||
|
|
||
| # Should keep the new parameter value | ||
| assert ( | ||
| result.get("preserve_info_file") is False | ||
| ), "New preserve_info_file parameter should take precedence" | ||
| assert "cleanup_on_exit" not in result, "cleanup_on_exit should be removed" | ||
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.
How shall I call the launcher now if I don't want to shut down the Fluent process on
session.exit()?Uh oh!
There was an error while loading. Please reload this page.
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.
@mainakk If I understood properly your point then we can preserve the server info file and avoid cleanup on session.exit() , call the launcher with
session = pyfluent.launch_fluent(preserve_info_file = True)session.exit()Kindly let me know, if I misunderstood your question or any further query or any change needed.
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 is intended above.
Uh oh!
There was an error while loading. Please reload this page.
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.
@mayankansys is responding to my comment above but that comment does not request any changes. If we agree to follow up on that idea, we would create a separate issue.