Conversation
michaelbynum
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3701 +/- ##
==========================================
- Coverage 89.94% 86.76% -3.18%
==========================================
Files 902 902
Lines 106457 106595 +138
==========================================
- Hits 95748 92492 -3256
- Misses 10709 14103 +3394
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:
|
jsiirola
left a comment
jsiirola
left a comment
There was a problem hiding this comment.
A couple minor questions that don't need to hold up the PR and one significant one that probably does (it look like the ASL solution loader is not loading all import suffixes).
| vars_to_load: list | ||
| A list of the variables whose solution value should be retrieved. If vars_to_load | ||
| is None, then the values for all variables will be retrieved. | ||
| solution_id: Optional[Any] |
There was a problem hiding this comment.
Instead of Optional[Type], I believe the current "preferred Python" annotation is Type | None. In this case, since Any includes None, I think you could just do Any (although Any | None is a little more explicit).
| def load_import_suffixes(self, solution_id=None): | ||
| load_import_suffixes(self._pyomo_model, self, solution_id=solution_id) |
There was a problem hiding this comment.
This only loads duals and rc ... the ASL supports arbitrary inpirt suffixes, which I think need to be supported here.
There was a problem hiding this comment.
Can you give me an example of what ASLSolFileData would look like if it had other suffixes. I see that it has attributes like var_suffixes. I assume the keys are the names of the suffixes, and the values map the variable indices to the suffix values? Any suggestions on how to test this?
There was a problem hiding this comment.
Yes: Suffix data comes back from the ASL flagged for "variables", "constraints", "objectives", and the "problem". We blindly parse that into the corresponding attributes (var_suffixes, etc).
- For
var,con, andobj, the dict maps suffix name to a dict that maps the 0-based integer index of the relevant variable / constraint / objective in the corresponding list in theNLWriterInforeturned by the writer tot he suffix value. - For the problem, since there is only one problem, the
problem_suffixesdict just maps suffix name to value.
There was a problem hiding this comment.
Done - kind of. I just raise an error if any variables are eliminated or any scaling is done.
|
@AnhTran01 , @boxblox - could you please take a look at the changes to the gams solution loader? |
yep, is there anything in particular you need me to focus in on? on my first pass it looks like a lot of internal organization. Not sure I'd have much to add on that, but can look for an eye for clarity, if that's helpful. |
I mostly wanted to make sure I understood when a solution is available for the purposes of |
|
@jsiirola - this should be ready for a follow-up review. One thing that is probably worth further discussion: I added a method to the |
|
The Jenkins failures seem to be unrelated to this PR. They look like timeout error during calls to |
| from __future__ import annotations | ||
|
|
||
| from typing import Sequence, Dict, Optional, Mapping, List, Any |
There was a problem hiding this comment.
Do you know what you need from __future__ here?
Also, as we are Python>-3.10, you don't need List or Dict (and can just use list and dict)
|
|
||
| from collections.abc import Mapping, Sequence | ||
| from typing import Protocol | ||
| from typing import Any, List, Protocol |
There was a problem hiding this comment.
Again, you don't need to import List and can just use list
| from pyomo.core.base.suffix import Suffix | ||
|
|
||
|
|
||
| def load_import_suffixes( |
There was a problem hiding this comment.
There's a couple problems with this implementation:
- this doesn't clear any import suffixes other than
dualandrc - if there are multiple dual suffixes, only the last one found is cleared (and that is usually the deeper one in the tree)
See the next comment for a proposed solution
| If there are multiple solutions, this specifies which solution | ||
| should be loaded. If None, the default solution will be used. | ||
| """ | ||
| return NotImplemented |
There was a problem hiding this comment.
I propose the following default implementation:
| return NotImplemented | |
| def load_import_suffixes(self, solution_id=None): | |
| """Clear import suffixes on the model and load data returned by the solver. | |
| Parameters | |
| ---------- | |
| solution_id: Optional[Any] | |
| If there are multiple solutions, this specifies which solution | |
| should be loaded. If None, the default solution will be used. | |
| """ | |
| suffixes = self._clear_import_suffixes() | |
| if 'dual' in known_suffixes: | |
| suffixes['dual'].update(solution_loader.get_duals(solution_id=solution_id)) | |
| if 'rc' in known_suffixes: | |
| suffixes['rc'].update( | |
| solution_loader.get_reduced_costs(solution_id=solution_id) | |
| ) | |
| def _clear_import_suffixes(self): | |
| """Clear and return all import suffixes on the model | |
| This walks the Pyomo model and clears all :class:`Suffix` | |
| components that are flagged to import values from the solver | |
| (this includes :attr:`Suffix.IMPORT` and | |
| :attr:`Suffix.IMPORT_EXPORT`). It returns a :class:`dict` | |
| mapping the :attr:`Suffix.local_name` to the :class:`Suffix` | |
| closest to the root block. | |
| Returns | |
| ------- | |
| known_suffixes : dict[str, Suffix] | |
| """ | |
| known_suffixes = {} | |
| for suffix in self.pyomo_model.component_objects( | |
| Suffix, | |
| active=True, | |
| descend_into=True, | |
| descent_order=TraversalStrategy.BreadthFirstSearch, | |
| ): | |
| if not suffix.import_enabled(): | |
| continue | |
| suffix.clear() | |
| known_suffixes.setdefault(suffix.local_name, suffix) | |
| return known_suffixes |
This resolves issues with not clearing all import suffixes and handling multiple Suffix components with the same name. It also makes this a proper method of the loader (the "public utility function" seemed a bit strange).
This does imply a more significant change: it requires all loaders to keep an explicit reference to the Pyomo model. It is an open question if pyomo_model should just be a public attribute or a property... Either way, I think that is actually a positive change, but it is a bigger one that we should at least acknowledge/discuss.
There was a problem hiding this comment.
An alternate implementation could be to force the solver to pre-collect the import Suffixes and just record that list on the SolutionLoader, but that seems more clunky and repetitive (each solver would have to remember to implement it)?
| """ | ||
| return NotImplemented | ||
|
|
||
| def load_solution(self, solution_id=None): |
There was a problem hiding this comment.
I have a general question about this design: do we know the "cost" of switching the "active solution"? As implemented, load_solution will set and clear the active solution up to 3 times (for vars, duals, and reduced costs). Do we know if that is a problem.
An alternate solution could be to implement a context manager, so users could do things like:
results = solver.solve(m)
# ...
solution_ids = results.get_solution_ids()
with results.solution(solution_ids[2]):
results.load_solution()
# or
vars = results.get_vars()
duals = results.get_duals()I am not proposing a change (yet) ... just interested in your thoughts.
There was a problem hiding this comment.
One advantage of the context manager approach is we would only need to validate the solution_id in one place (instead of repeating it in every load / get method).
There was a problem hiding this comment.
Currently, the only interfaces that actually support solution pools are the gurobi interfaces. With those, I don't think there is really any cost. That being said, I still like your suggestion - particularly the bit about not checking the solution id in every method...
| def get_solution_ids(self) -> List[Any]: | ||
| return [None] |
There was a problem hiding this comment.
Should this return [] if self._nl_info is None?
There was a problem hiding this comment.
Oh, good catch. I need to add a test for this, too.
There was a problem hiding this comment.
I did modify this. There are some lingering issues, but I would prefer to address them after #3703 is merged. I added an issue to track this (#3917). Right now, I don't think it is possible to hit the case where _nl_info is None. What happens is that results.solution_loader ends up being None instead. I personally would find that unexpected...
5 participants
Summary/Motivation:
This PR updates the solution loader in pyomo/contrib/solver based on recent design discussions.
Changes proposed in this PR:
get_number_of_solutionsmethodget_solution_idsmethodload_solution_methodload_import_suffixesmethodsolution_idas an argument to methodsLegal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: