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

Refactor misc.py #1276

Merged
merged 9 commits into from
Nov 27, 2023
Merged

Refactor misc.py #1276

merged 9 commits into from
Nov 27, 2023

Conversation

PProfizi
Copy link
Contributor

@PProfizi PProfizi commented Nov 15, 2023

Refactor misc.py/_find_latest_ansys_versions as the same logic is now at the gate level in load_api.py.

Originally showed the required changes on pygate to close #1273

@PProfizi PProfizi added the enhancement New feature or request label Nov 15, 2023
@PProfizi PProfizi self-assigned this Nov 15, 2023
@PProfizi PProfizi requested a review from a team as a code owner November 15, 2023 14:21
Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Merging #1276 (dd46026) into master (8048e2b) will increase coverage by 0.21%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1276      +/-   ##
==========================================
+ Coverage   86.16%   86.37%   +0.21%     
==========================================
  Files          81       81              
  Lines        9251     9229      -22     
==========================================
+ Hits         7971     7972       +1     
+ Misses       1280     1257      -23     

src/ansys/dpf/gate/load_api.py Outdated Show resolved Hide resolved
src/ansys/dpf/gate/load_api.py Outdated Show resolved Hide resolved
if not os.path.isdir(ansys_path):
continue
# Check that it contains a DPF install
if not os.path.exists(os.path.join(ansys_path, _get_path_in_install())):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_get_path_in_install should be enriched to also check the existence of the dpf folder, where the majority of libraries are now placed

Copy link
Contributor Author

@PProfizi PProfizi Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rafacanton we could, yet checks are usually made in methods which call _get_path_in_install:

That would mean refactoring all of those.
Still I agree that the current checks may not be enough. We should check that a given list of DLLs (TBD) is actually present.
This can be added to the objectives of this issue/PR, what do you think?

Copy link
Contributor Author

@PProfizi PProfizi Nov 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rafacanton while implementing a test for dpf/bin/winx64 or dpf/bin/linx64 folder, I realized that we may have an issue in ServerFactory.get_server_type_from_config which basically enforces use of AWP_ROOT242 when in non-legacy Grpc to define the aisol path to add to PATH... I think this is quite a problem. I also noticed that we still use _find_outdated_ansys_version() which tests for Ansys versions below 221... I do not think we still need that as it was used to force LegacyGrpc connections for servers 221 or below while now we could argue that the client itself does not support servers below 222. If we allow this connection to happen, there WILL be failures anyway.

I think there may be a lot of refactoring to do here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: adding the test for a dpf folder, yet it appears consistently only starting with 231.

@PProfizi PProfizi changed the title Check AWP_ROOTXXX paths Refactor misc.py Nov 24, 2023
@PProfizi PProfizi merged commit de0a677 into master Nov 27, 2023
33 of 36 checks passed
@PProfizi PProfizi deleted the fix/check_awp_root_path branch November 27, 2023 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check AWP_ROOT path for DPF install
2 participants