-
Notifications
You must be signed in to change notification settings - Fork 561
TYP: Enhance static typing #3771
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
base: main
Are you sure you want to change the base?
Conversation
jsiirola
left a comment
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.
Thanks for this! I have several questions (see below). Some are less about this PR specifically, and more about generally how we want to better support typing in Pyomo. It would be useful to talk through that sometime in a Tuesday Dev Call.
pyomo/common/pyomo_typing.py
Outdated
| if typing.TYPE_CHECKING: | ||
| from typing import overload as overload | ||
| else: |
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.
Why do you need to disable the overload?
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.
A function calling typing.overload is not recognized as an overload by the type checker.
We need to replace it with typing.overload during type checking.
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.
OK. I went digging and it turns out that logic is primarily needed by a dependent project -- but only for Python versions through 3.10. I think we should actually document that better in the code, with something like:
if sys.version_info[:2] <= (3, 10) and not TYPE_CHECKING:
def overload(func: typing.Callable):
"""Wrap typing.overload that remembers the overloaded signatures
This provides a custom implementation of typing.overload that
remembers the overloaded signatures so that they are available for
runtime inspection (backporting `get_overloads` from Python 3.11+).
"""
_overloads.setdefault(_get_fullqual_name(func), []).append(func)
return typing.overload(func)
def get_overloads_for(func: typing.Callable):
return _overloads.get(_get_fullqual_name(func), [])
else:
from typing import overload, get_overloads as get_overloads_forThere 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.
typing_extensions might be helpful.
pyomo/core/base/PyomoModel.py
Outdated
|
|
||
|
|
||
| # NOTE: Python 3.11+ use `typing.Self` | ||
| ModelT = TypeVar("ModelT", bound="Model") |
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.
We haven't really settled on a typing convention for naming types, but appending T seems confusing.
- would standardizing on appendint
Typebe more clear? - should local TypeVar objects be private by default (i.e.,
_modelTypehere)?
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.
_ModelType is better.
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.
Agreed.
pyomo/future.py
Outdated
| for ver, cls in versions.items(): | ||
| if cls._cls is _environ.SolverFactory._cls: | ||
| solver_factory._active_version = ver | ||
| solver_factory._active_version = ver # type: ignore |
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.
We shouldn't need these annotations, should we?
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.
Because we cannot add attributes to FunctionType.
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.
You can't annotate the function, plus the attribute whereit is first instantiated at the bottom of the file?
def solver_factory(version: int | None = None) -> int:
# ...
solver_factory._active_version: int = solver_factory()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.
It is allowed at runtime, but it's a type violation.
ref: microsoft/pyright#8838
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3771 +/- ##
=======================================
Coverage 89.41% 89.42%
=======================================
Files 909 909
Lines 105579 105589 +10
=======================================
+ Hits 94407 94421 +14
+ Misses 11172 11168 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
We were talking though this PR on the dev call this week. I think there are 3 changes that need to be made to get it merged:
|
Co-Authored-By: John Siirola <356359+jsiirola@users.noreply.github.com>
|
Almost done.
Is this intended to be public? |
jsiirola
left a comment
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.
Some minor comments. The biggest is that I would prefer to see the changes to pyomo.future.solver_factory() be reverted with the exception of promoting the _active_version attribute on the function to a _active_solver_factory_version module attribute. The changes add an unnecessary (admittedly slight) overhead and break some of the documented functionality.
pyomo/future.py
Outdated
| solver_factory_v1: _SolverFactoryClassV1 = _solvers.LegacySolverFactory | ||
| solver_factory_v2: _SolverFactoryClassV2 = _appsi.SolverFactory | ||
| solver_factory_v3: _SolverFactoryClassV3 = _contrib.SolverFactory |
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.
We don't want to declare these at the module scope: if we do, the module __getattr__ won't be called, which was how we were implementing the Python idiom:
from pyomo.__future__ import solver_factory_v3as an alias for
from pyomo.__future__ import solver_factory
solver_factory(3)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.
My bad, it's regression.
I didn't realize that importing changes the state.
pyomo/future.py
Outdated
| solver_factory_v2: _SolverFactoryClassV2 = _appsi.SolverFactory | ||
| solver_factory_v3: _SolverFactoryClassV3 = _contrib.SolverFactory | ||
|
|
||
| _versions = {1: solver_factory_v1, 2: solver_factory_v2, 3: solver_factory_v3} |
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.
Future is intended to manage all "future compatibility changes", so we should use a more specific name than versions here - there will eventually be several "versions" that this module will manage.
I would prefer not promoting this dict to the module scope: solver_factory will probably never be called more than once, so the performance gain of preserving the dict isn't worth the memory.
pyomo/future.py
Outdated
| _SolverFactoryClassV1 = _solvers.SolverFactoryClass | ||
| _SolverFactoryClassV2 = _appsi.SolverFactoryClass | ||
| _SolverFactoryClassV3 = _contrib.SolverFactoryClass |
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.
I would prefer not caching these at the module scope.
… variable" This reverts commit b5a45cb.
jsiirola
left a comment
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.
This is super close. If we can delete the 3 lines indicated, then I think this is good to go.
pyomo/future.py
Outdated
| _active_solver_factory_version = ver | ||
| break | ||
| return solver_factory._active_version | ||
| return _active_solver_factory_version |
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.
[This is my fault] As I am thinking through this, I think this is actually a bug and this should be reworked. Right now, everything is "fine" because we are guaranteed that the first time this function is called, it will be called with version=None (that is hard-coded below and will happen on import). BUT that seems fragile. I think I would propose combining this logic with the if version is None below:
if version is None:
if "_active_solver_factory_version" not in globals():
for ver, cls in versions.items():
if cls._cls is _environ.SolverFactory._cls:
_active_solver_factory_version = ver
break
return _active_solver_factory_version@n-takumasa: totally up to you if you want to make this change in this PR or leave it to me to get back to later
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.
I agree because if we delete _active_solver_factory_version = solver_factory() and call solver_factory(int), it will break.
Co-Authored-By: John Siirola <356359+jsiirola@users.noreply.github.com>
Fixes # NA
Summary/Motivation:
To provide better autocompletion in vscode+pylance.
Complex dynamic patterns (e.g.
Var()can returnScalarVarorIndexedVar) are out of scope as they are difficult to model accurately.Changes proposed in this PR:
typing.overloadduringTYPE_CHECKINGModel.__new__should return an instance of the subclassSolverFactorys andpyomo.future.solver_factory@overloads, overlapped overloads never be usedLegal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: