Skip to content

Fix boolean conversion in DeferredImportIndicator class#3746

Merged
jsiirola merged 2 commits intoPyomo:mainfrom
eminyouskn:deferred-import-indicator-bool
Oct 11, 2025
Merged

Fix boolean conversion in DeferredImportIndicator class#3746
jsiirola merged 2 commits intoPyomo:mainfrom
eminyouskn:deferred-import-indicator-bool

Conversation

@eminyouskn
Copy link
Copy Markdown
Contributor

@eminyouskn eminyouskn commented Oct 7, 2025

Fixes # NA

Summary/Motivation:

Some linters, such as Pyright, raise warnings when an object of class DeferredImportIndicator is used in an if statement, since its __bool__ method may return None.
This PR explicitly casts the result of that method to bool to ensure a proper boolean value is returned.

Changes proposed in this PR:

  • Cast the result of __bool__ method of DeferredImportIndicator to be a bool

Legal Acknowledgement

By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the BSD license.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

@eminyouskn eminyouskn mentioned this pull request Oct 7, 2025
Merged
@jsiirola
Copy link
Copy Markdown
Member

jsiirola commented Oct 7, 2025

This is a nonfunctional change in that _available will always be bool after calling resolve(). I am curious: If you implement this change instead, does the linter recognize that fact?

diff --git a/pyomo/common/dependencies.py b/pyomo/common/dependencies.py
index 27167f12f..017b76c2c 100644
--- a/pyomo/common/dependencies.py
+++ b/pyomo/common/dependencies.py
@@ -344,13 +344,13 @@ class DeferredImportIndicator(_DeferredImportIndicatorBase):
         if callback is not None:
             DeferredImportCallbackFinder._callbacks.setdefault(name, []).append(self)

-    def __bool__(self):
+    def __bool__(self) -> bool:
         self.resolve()
         return self._available

     def resolve(self):
         # Only attempt the import once, then cache some form of result
-        if self._module is None:
+        if self._available is None:
             package = self._original_globals.get('__name__', '')
             try:
                 self._module, self._available = _perform_import(
(

@eminyouskn
Copy link
Copy Markdown
Contributor Author

eminyouskn commented Oct 7, 2025

Even with the proposed changes it does not managed to infer that _available has been edited to be a bool in the resolve function. If you don't like the bool casting, how about create another flag _import_attempted and from the begining put _available to False et not to None? I assume that the None status of _available is only to know if the import has been attempted or not.

jsiirola
jsiirola reviewed Oct 9, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@jsiirola jsiirola left a comment

Choose a reason for hiding this comment

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

OK - I have thought about this a lot. The lack of a cast was previously defensive programming so that if - for whatever reason - resolve() failed to set _available to a bool, then Python would raise an exception. I think I would still like that behavior, so I suggest we add an assertion and then a couple comments explaining why things look weird.

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 10, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.32%. Comparing base (e8ed5dd) to head (e27bd21).
⚠️ Report is 595 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #3746   +/-   ##
=======================================
  Coverage   89.31%   89.32%           
=======================================
  Files         896      896           
  Lines      103697   103698    +1     
=======================================
+ Hits        92619    92626    +7     
+ Misses      11078    11072    -6
Flag Coverage Δ
builders 29.09% <100.00%> (+<0.01%) ⬆️
default 85.93% <100.00%> (?)
expensive 35.86% <100.00%> (?)
linux 86.96% <100.00%> (-2.09%) ⬇️
linux_other 86.96% <100.00%> (+<0.01%) ⬆️
osx 83.10% <100.00%> (+<0.01%) ⬆️
win 85.23% <100.00%> (+<0.01%) ⬆️
win_other 85.23% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jsiirola jsiirola merged commit f029e34 into Pyomo:main Oct 11, 2025
92 of 95 checks passed
@eminyouskn eminyouskn deleted the deferred-import-indicator-bool branch October 11, 2025 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jsiirola jsiirola jsiirola approved these changes

@mrmundt mrmundt mrmundt approved these changes

Assignees

No one assigned

Labels

AT: PRE-TEST INSPECTED

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@eminyouskn @jsiirola @mrmundt @pyomo-autotest