Skip to content

Conversation

@koubaa
Copy link
Contributor

@koubaa koubaa commented May 1, 2025

Attempting to import a module to check if it is installed is unnecessary. If there is a problem with a module that is installed, it will manifest on import and not on use, which can lead to errors even when that module is never used.

This doesn't fully fix the problem because _apply_default_theme is still used even when plotting isn't done

@koubaa koubaa requested a review from a team as a code owner May 1, 2025 12:20
@koubaa koubaa requested review from germa89 and pyansys-ci-bot May 1, 2025 12:20
@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

@koubaa koubaa changed the title change how the install check works refactor: change how the install check works May 1, 2025
@github-actions github-actions bot added the enhancement Improve any current implemented feature label May 1, 2025
@codecov
Copy link

codecov bot commented May 1, 2025

Codecov Report

Attention: Patch coverage is 96.55172% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.28%. Comparing base (1ec7e5c) to head (40d6c9e).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3887      +/-   ##
==========================================
- Coverage   88.29%   88.28%   -0.02%     
==========================================
  Files         187      187              
  Lines       14819    14830      +11     
==========================================
+ Hits        13084    13092       +8     
- Misses       1735     1738       +3     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@germa89
Copy link
Collaborator

germa89 commented May 5, 2025

@koubaa it seems you still need the try and except context manager:

https://github.com/ansys/pymapdl/actions/runs/14777942416/job/41491532825?pr=3887#step:10:92

GitHub
A Python client library for Ansys MAPDL. Contribute to ansys/pymapdl development by creating an account on GitHub.

@germa89
Copy link
Collaborator

germa89 commented May 5, 2025

@koubaa

Thanks to your PR I realised that there a bit of code duplicity. I will take care of that in a follow up PR. Additionally what I meant with cache is what is shown here:

def is_package_installed_cached(package_name):

functools.cache can help us to cache functions so we only checks import (an their try/except once).

@germa89
Copy link
Collaborator

germa89 commented May 5, 2025

You might want to remove this:

if _HAS_VISUALIZER:
import pyvista as pv
from ansys.mapdl.core.plotting.plotting_defaults import DefaultSymbol
BC_plot_settings = DefaultSymbol()

@koubaa koubaa changed the title refactor: change how the install check works refactor: do not import ansys.tools.visualizer by default when importing ansys.mapdl.core May 5, 2025
@koubaa
Copy link
Contributor Author

koubaa commented May 5, 2025

This change has grown because the visualization interface is loaded on startup in multiple places, and I've modified it not to load on startup at all. This may break some scripts, but none of the examples we distribute are affected

@germa89
Copy link
Collaborator

germa89 commented May 6, 2025

Ping me when this is ready (CICD is failing at the moment)

@github-actions github-actions bot added the documentation Documentation related (improving, adding, etc) label May 6, 2025
@koubaa
Copy link
Contributor Author

koubaa commented May 6, 2025

@germa89 I fixed the tests but there is one failure in autodoc that I don't understand:

WARNING: autodoc: failed to import class 'MapdlPlotter' from module 'ansys.mapdl.core.plotting'; the following exception was raised:

I made sure that all the references to MapdlPlotter use the full namespace in the pymapdl repo. Does autodoc pull from other repos too?

@germa89
Copy link
Collaborator

germa89 commented May 6, 2025

@koubaa the documentation failure was due to using previous build cache. I cleaned it up using this workflow and now it seems fine.

Additionally, codacy is showing this warning, but I guess it is fine... so I will disable it.

image

Copy link
Collaborator

@germa89 germa89 left a comment

Choose a reason for hiding this comment

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

I am not particularly happy with having variables in a module to be initialized after the first plot.

from ansys.mapdl.core.plotting import MapdlPlotter
from ansys.mapdl.core.plotting import BC_plot_settings # Fails
# plot somethin
pl = MapdlPlotter()
from ansys.mapdl.core.plotting import BC_plot_settings # DOES NOT Fails

Which is why codacy raise the issue. This behaviour to me is an antipattern. However, if this is the only way to get rid of that bug we mentioned, let it be until fix that upstream.

== EDIT ==

Maybe we can just define BC_plot_settings in the module globally as None or a non-initialized class... So we get rid of the codacy warning, and we have a consistent module API.

koubaa and others added 2 commits May 6, 2025 11:02
* hold the bc settings per plotter instance

* ci: auto fixes from pre-commit.com hooks.

for more information, see https://pre-commit.ci

* chore: adding changelog file 3897.miscellaneous.md [dependabot-skip]

---------

Co-authored-by: Mohamed Koubaa <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: pyansys-ci-bot <[email protected]>
@germa89 germa89 self-requested a review May 6, 2025 16:40
@koubaa koubaa merged commit 1b1b6b4 into main May 7, 2025
86 of 87 checks passed
@koubaa koubaa deleted the check-installed-without-import branch May 7, 2025 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Documentation related (improving, adding, etc) enhancement Improve any current implemented feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants