-
Notifications
You must be signed in to change notification settings - Fork 560
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?
Changes from all commits
4389d6d
4765c4a
15528c3
360064e
4be9b33
58ec35d
fb6859f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |
| from weakref import ref as weakref_ref | ||
| import gc | ||
| import math | ||
| from typing import TypeVar | ||
|
|
||
| from pyomo.common import timing | ||
| from pyomo.common.collections import Bunch | ||
|
|
@@ -572,6 +573,10 @@ def select( | |
| StaleFlagManager.mark_all_as_stale(delayed=True) | ||
|
|
||
|
|
||
| # NOTE: Python 3.11+ use `typing.Self` | ||
| ModelT = TypeVar("ModelT", bound="Model") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. |
||
|
|
||
|
|
||
| @ModelComponentFactory.register( | ||
| 'Model objects can be used as a component of other models.' | ||
| ) | ||
|
|
@@ -583,9 +588,9 @@ class Model(ScalarBlock): | |
|
|
||
| _Block_reserved_words = set() | ||
|
|
||
| def __new__(cls, *args, **kwds): | ||
| def __new__(cls: type[ModelT], *args, **kwds) -> ModelT: | ||
| if cls != Model: | ||
| return super(Model, cls).__new__(cls) | ||
| return super(Model, cls).__new__(cls) # type: ignore | ||
|
|
||
| raise TypeError( | ||
| "Directly creating the 'Model' class is not allowed. Please use the " | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,8 +9,15 @@ | |
| # This software is distributed under the 3-clause BSD License. | ||
| # ___________________________________________________________________________ | ||
|
|
||
| from typing import TYPE_CHECKING, Any, Literal, overload | ||
|
|
||
| import pyomo.environ as _environ | ||
|
|
||
| if TYPE_CHECKING: | ||
| import pyomo.contrib.appsi.base as _appsi | ||
| import pyomo.contrib.solver.common.factory as _contrib | ||
| import pyomo.opt.base.solvers as _solvers | ||
|
|
||
| __doc__ = """ | ||
| Preview capabilities through ``pyomo.__future__`` | ||
| ================================================= | ||
|
|
@@ -28,13 +35,29 @@ | |
|
|
||
| """ | ||
|
|
||
| solver_factory_v1: "_solvers.SolverFactoryClass" | ||
| solver_factory_v2: "_appsi.SolverFactoryClass" | ||
| solver_factory_v3: "_contrib.SolverFactoryClass" | ||
|
|
||
|
|
||
| def __getattr__(name): | ||
| if name in ('solver_factory_v1', 'solver_factory_v2', 'solver_factory_v3'): | ||
| if name in ("solver_factory_v1", "solver_factory_v2", "solver_factory_v3"): | ||
| return solver_factory(int(name[-1])) | ||
| raise AttributeError(f"module '{__name__}' has no attribute '{name}'") | ||
|
|
||
|
|
||
| @overload | ||
| def solver_factory(version: None = None) -> int: ... | ||
| @overload | ||
| def solver_factory(version: Literal[1]) -> "_solvers.SolverFactoryClass": ... | ||
| @overload | ||
| def solver_factory(version: Literal[2]) -> "_appsi.SolverFactoryClass": ... | ||
| @overload | ||
| def solver_factory(version: Literal[3]) -> "_contrib.SolverFactoryClass": ... | ||
| @overload | ||
| def solver_factory(version: int) -> Any: ... | ||
|
|
||
|
|
||
| def solver_factory(version=None): | ||
| """Get (or set) the active implementation of the SolverFactory | ||
|
|
||
|
|
@@ -90,19 +113,19 @@ def solver_factory(version=None): | |
| if current is None: | ||
| 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We shouldn't need these annotations, should we?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because we cannot add attributes to
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is allowed at runtime, but it's a type violation. |
||
| break | ||
| return solver_factory._active_version | ||
| return solver_factory._active_version # type: ignore | ||
| # | ||
| # The user is just asking what the current SolverFactory is; tell them. | ||
| if version is None: | ||
| return solver_factory._active_version | ||
| return solver_factory._active_version # type: ignore | ||
| # | ||
| # Update the current SolverFactory to be a shim around (shallow copy | ||
| # of) the new active factory | ||
| src = versions.get(version, None) | ||
| if version is not None: | ||
| solver_factory._active_version = version | ||
| solver_factory._active_version = version # type: ignore | ||
| for attr in ('_description', '_cls', '_doc'): | ||
| setattr(_environ.SolverFactory, attr, getattr(src, attr)) | ||
| else: | ||
|
|
@@ -113,4 +136,4 @@ def solver_factory(version=None): | |
| return src | ||
|
|
||
|
|
||
| solver_factory._active_version = solver_factory() | ||
| solver_factory._active_version = solver_factory() # type: ignore | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |||||||||||||||||||||
| import time | ||||||||||||||||||||||
| import logging | ||||||||||||||||||||||
| import shlex | ||||||||||||||||||||||
| from typing import overload | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| from pyomo.common import Factory | ||||||||||||||||||||||
| from pyomo.common.enums import SolverAPIVersion | ||||||||||||||||||||||
|
|
@@ -144,6 +145,11 @@ def _solver_error(self, method_name): | |||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| class SolverFactoryClass(Factory): | ||||||||||||||||||||||
| @overload | ||||||||||||||||||||||
| def __call__(self, _name: None = None, **kwds) -> "SolverFactoryClass": ... | ||||||||||||||||||||||
| @overload | ||||||||||||||||||||||
| def __call__(self, _name, **kwds) -> "OptSolver": ... | ||||||||||||||||||||||
|
Comment on lines
+148
to
+151
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Python documentation indicates that you should only use the
Suggested change
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not believe the documentation states that you must not use |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def __call__(self, _name=None, **kwds): | ||||||||||||||||||||||
| if _name is None: | ||||||||||||||||||||||
| return self | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
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.overloadis not recognized as an overload by the type checker.We need to replace it with
typing.overloadduring 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:
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.
typing_extensions might be helpful.