Skip to content
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

FIX: Loading default settings value #5531

Merged
merged 12 commits into from
Dec 10, 2024

Conversation

SMoraisAnsys
Copy link
Collaborator

@SMoraisAnsys SMoraisAnsys commented Dec 5, 2024

Description

This changes should allow one to load the default pyaedt_settings.yaml file.
Previously, if a user tried to use the file as it is and only modified one setting, e.g. enable_screen_logs, then loading the YAML file failed because some fields are casted / tested on the fly.

Also, while working on this and testing, I found that we were not correctly hanling the update of the AEDT environment variables stored in Settings. This PR also fixes it.

Close #5536

Pinging @kruglovIFX for visibility. Note that you'll have to wait for this to be merged and a new release to published in order to leverage this fix.

@ansys-reviewer-bot
Copy link
Contributor

Thanks for opening a Pull Request. If you want to perform a review write a comment saying:

@ansys-reviewer-bot review

@github-actions github-actions bot added the bug Something isn't working label Dec 5, 2024
Copy link

codecov bot commented Dec 5, 2024

Codecov Report

Attention: Patch coverage is 89.28571% with 3 lines in your changes missing coverage. Please review.

Project coverage is 84.85%. Comparing base (e62d2a4) to head (02a0301).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5531      +/-   ##
==========================================
+ Coverage   84.75%   84.85%   +0.09%     
==========================================
  Files         144      144              
  Lines       60189    60624     +435     
==========================================
+ Hits        51011    51440     +429     
- Misses       9178     9184       +6     

@SMoraisAnsys SMoraisAnsys self-assigned this Dec 6, 2024
@SMoraisAnsys SMoraisAnsys marked this pull request as ready for review December 6, 2024 09:00
@SMoraisAnsys
Copy link
Collaborator Author

@Samuelopez-ansys @Alberto-DM Would it make sense to add ANS_NODEPCHECK=1 by default even if we are not in Windows environment ?

I'm asking that because if a user tries to come up with his own default settings file, he might start from the one we provide as an example. However, this file does not contain ANS_NODEPCHECK. The reason for that is that this environment variable seems to be leveraged in Linux environment but not in Windows. Therefore, users might end up with a configuration were ANS_NODEPCHECK is not set :/

@Alberto-DM
Copy link
Contributor

@Samuelopez-ansys @Alberto-DM Would it make sense to add ANS_NODEPCHECK=1 by default even if we are not in Windows environment ?

I'm asking that because if a user tries to come up with his own default settings file, he might start from the one we provide as an example. However, this file does not contain ANS_NODEPCHECK. The reason for that is that this environment variable seems to be leveraged in Linux environment but not in Windows. Therefore, users might end up with a configuration were ANS_NODEPCHECK is not set :/

If the env variable ANS_NODEPCHECK is ignored in Windows, we can keep it always set to 1.

@SMoraisAnsys
Copy link
Collaborator Author

@Samuelopez-ansys @Alberto-DM Would it make sense to add ANS_NODEPCHECK=1 by default even if we are not in Windows environment ?
I'm asking that because if a user tries to come up with his own default settings file, he might start from the one we provide as an example. However, this file does not contain ANS_NODEPCHECK. The reason for that is that this environment variable seems to be leveraged in Linux environment but not in Windows. Therefore, users might end up with a configuration were ANS_NODEPCHECK is not set :/

If the env variable ANS_NODEPCHECK is ignored in Windows, we can keep it always set to 1.

The problem is that I don't know the impact of ANS_NODEPCHECK in Windows environment. Since it was only set for Linux, I assumed it would have some impact on Windows. @Samuelopez-ansys Do we have a way to check if there is any impact ? If that's the case, please let me know and I'll do the test myself.

@Samuelopez-ansys
Copy link
Member

@SMoraisAnsys I will check internally if it has any impact in Windows.

@Samuelopez-ansys
Copy link
Member

@SMoraisAnsys Confirmed. ANS_NODEPCHECK has only impact in Linux

@SMoraisAnsys
Copy link
Collaborator Author

@Samuelopez-ansys @gmalinve While working on improving code coverage, I discovered that block_figure_plot was not handled with YAML loading / dumping. It got through #5297 without being noticed.
This ping is just a reminder that if you ever add a new setting or see a PR doing so, we need to update the loading of YAML files.
Note that I added a test that should fail if one doesn't do it :D

@SMoraisAnsys SMoraisAnsys merged commit 8c4519a into main Dec 10, 2024
43 checks passed
@SMoraisAnsys SMoraisAnsys deleted the fix/settings-loading-default-file branch December 10, 2024 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve settings to work with default YAML file
3 participants