-
Notifications
You must be signed in to change notification settings - Fork 56
fix: example deprecation warnings and improvements #4512
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
161dd03 to
376508f
Compare
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 fixes access to solver session settings throughout the test suite and examples. The changes update all references from direct access patterns (e.g., solver.setup.models) to the correct settings-based access pattern (e.g., solver.settings.setup.models). This is a comprehensive update ensuring consistent API usage across the entire codebase.
- Updates solver session settings access pattern in tests and examples
- Adds missing
.settingsprefix to all solver session attribute access - Minor formatting improvements and code cleanup
Reviewed Changes
Copilot reviewed 69 out of 70 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| tests/*.py | Updated solver session settings access to use .settings prefix |
| examples/*.py | Updated solver session settings access pattern in all examples |
| src/ansys/fluent/core/*.py | Updated settings access in core modules |
| doc/source/*.rst | Updated documentation examples to use correct settings API |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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 68 out of 69 changed files in this pull request and generated 6 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
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 68 out of 69 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@Gobot1234 Please could you use the above overview as a basis for your PR description. |
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 68 out of 69 changed files in this pull request and generated 7 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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 66 out of 67 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
tests/test_settings_api.py:1
- This error message refers to
<session>.settings.results.report.fluxes.mass_flowbut based on the other patterns in the codebase, it should be<session>.results.report.fluxes.mass_flowsince results don't go through the settings prefix.
# Copyright (C) 2021 - 2025 ANSYS, Inc. and/or its affiliates.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| s.split("\n")[-2].split("(")[0] | ||
| == r"<solver_session>.settings.file.read_case" | ||
| ) | ||
| assert s.split("\n")[-2].split("(")[0] == r"<solver_session>.file.read_case" |
Copilot
AI
Nov 3, 2025
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 assertion expects <solver_session>.file.read_case but based on the PR's goal of updating to settings-based access patterns, this should likely be <solver_session>.settings.file.read_case to match the updated API pattern.
| assert s.split("\n")[-2].split("(")[0] == r"<solver_session>.file.read_case" | |
| assert s.split("\n")[-2].split("(")[0] == r"<solver_session>.settings.file.read_case" |
| solver.tui.file.read_case(case_path) | ||
| timeout_loop( | ||
| lambda: "<solver_session>.settings.file.read_case" in capsys.readouterr().out, | ||
| lambda: "<solver_session>.file.read_case" in capsys.readouterr().out, |
Copilot
AI
Nov 3, 2025
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.
Similar to the previous comment, this assertion expects the old API pattern <solver_session>.file.read_case rather than the updated settings-based pattern <solver_session>.settings.file.read_case.
| lambda: "<solver_session>.file.read_case" in capsys.readouterr().out, | |
| lambda: "<solver_session>.settings.file.read_case" in capsys.readouterr().out, |
| solver_session.tui.define.parameters.enable_in_TUI("yes") | ||
| solver_session.settings.file.read_case(file_name=import_file_name) | ||
| solver_session.settings.solution.run_calculation.iter_count = 100 | ||
| solver_session.tui.define.settings.parameters.enable_in_TUI("yes") |
Copilot
AI
Nov 3, 2025
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 TUI path includes .settings. which appears inconsistent with other TUI usage patterns in the codebase. TUI commands typically don't go through the settings prefix.
| solver_session.tui.define.settings.parameters.enable_in_TUI("yes") | |
| solver_session.tui.define.parameters.enable_in_TUI("yes") |
|
|
||
| create_output_param = solver_session.tui.define.parameters.output_parameters.create | ||
| create_output_param = ( | ||
| solver_session.tui.define.settings.parameters.output_parameters.create |
Copilot
AI
Nov 3, 2025
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.
Similar issue as above - TUI commands should not include the .settings. prefix in their path.
|
Closing in favour of #4579 on this remote rather than mine |
Context
When solver session settings access was deprecated directly on solver the code wasn't updated I've also sprinkled in a couple of small improvements to the examples
Change Summary
Updated all references direct access patterns (
solver.setup.models) to the correct settings-based access pattern (solver.settings.setup.models) along with adding PEP 723 styles dependencies to the examples to make them easier to run as standalone scripts.Rationale
Fixes all the deprecation warnings
Impact
Changes basically everywhere