Skip to content
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

unlock pyomo version #124

Closed
wants to merge 2 commits into from

Conversation

PaulTalbot-INL
Copy link
Collaborator

@PaulTalbot-INL PaulTalbot-INL commented Jan 19, 2022


Pull Request Description

What issue does this change request address?

Continues #74

What are the significant changes in functionality due to this change request?

Unlocks the pyomo version in the dependency installer


For Change Control Board: Change Request Review

The following review must be completed by an authorized member of the Change Control Board.

  • 1. Review all computer code.
  • 2. If any changes occur to the input syntax, there must be an accompanying change to the user manual and xsd schema. If the input syntax change deprecates existing input files, a conversion script needs to be added (see Conversion Scripts).
  • 3. Make sure the Python code and commenting standards are respected (camelBack, etc.) - See on the wiki for details.
  • 4. Automated Tests should pass.
  • 5. If significant functionality is added, there must be tests added to check this. Tests should cover all possible options. Multiple short tests are preferred over one large tes.
  • 6. If the change modifies or adds a requirement or a requirement based test case, the Change Control Board's Chair or designee also needs to approve the change. The requirements and the requirements test shall be in sync.
  • 7. The merge request must reference an issue. If the issue is closed, the issue close checklist shall be done.
  • 8. If an analytic test is changed/added, the the analytic documentation must be updated/added.
  • 9. If any test used as a basis for documentation examples have been changed, the associated documentation must be reviewed and assured the text matches the example.

dylanjm
dylanjm previously approved these changes Jan 19, 2022
@PaulTalbot-INL
Copy link
Collaborator Author

PaulTalbot-INL commented Jan 19, 2022

The test didn't install a newer version of pyomo, and that bothers me a bit. Any thoughts?

EDIT: I actually see it pinned 5.7.0 still. I wonder if there's another plugin forcing that.

EDIT 2: it appears LOGOS is still pinning version, so this PR doesn't really test much, except to remove a potential future barrier. It can still be merged probably.

@dylanjm
Copy link
Collaborator

dylanjm commented Jan 19, 2022

it appears LOGOS is still pinning version, so this PR doesn't really test much, except to remove a potential future barrier. It can still be merged probably.

Do HERON tests pass locally on your machine with the updated pyomo version?

@PaulTalbot-INL
Copy link
Collaborator Author

PaulTalbot-INL commented Jan 19, 2022

Do HERON tests pass locally on your machine with the updated pyomo version?

That's a good question; my RAVEN is mid development right now so I can't check. I will soon.

UPDATE: It looks like there's some number-of-argument errors, investigating ...

UPDATE 2: An inner solve succeeds on pyomo 5.7.3. Checking other versions:

  • SUCCEEDS on 6.0
  • FAILS on 6.1
  • FAILS on 6.2

I don't see an obvious reason from the patchnotes why this changed, but I'm looking further.

The error raised is:

Traceback (most recent call last):
          File "/Users/talbpw/projects/raven/framework/Models/EnsembleModel.py", line 718, in __advanceModel
            evaluation = modelToExecute['Instance'].evaluateSample.original_function(modelToExecute['Instance'], origInputList, samplerType, inputKwargs)
          File "/Users/talbpw/projects/raven/framework/Models/ExternalModel.py", line 321, in evaluateSample
            result,instSelf = self._externalRun(inRun,)
          File "/Users/talbpw/projects/raven/framework/Models/ExternalModel.py", line 263, in _externalRun
            self.sim.run(externalSelf, InputDict)
          File "/Users/talbpw/projects/HERON/src/DispatchManager.py", line 700, in run
            dispatch, metrics = runner.run(raven_vars)
          File "/Users/talbpw/projects/HERON/src/DispatchManager.py", line 212, in run
            all_dispatch, metrics = self._do_dispatch(meta, all_structure, project_life, interp_years, segs, seg_type)
          File "/Users/talbpw/projects/HERON/src/DispatchManager.py", line 288, in _do_dispatch
            dispatch = self._dispatcher.dispatch(self._case, self._components, self._sources, meta)
          File "/Users/talbpw/projects/HERON/src/dispatch/pyomo_dispatch.py", line 196, in dispatch
            initial_levels, meta)
          File "/Users/talbpw/projects/HERON/src/dispatch/pyomo_dispatch.py", line 295, in dispatch_window
            self._create_objective(meta, m) # objective
          File "/Users/talbpw/projects/HERON/src/dispatch/pyomo_dispatch.py", line 659, in _create_objective
            m.obj = pyo.Objective(rule=rule, sense=pyo.maximize)
          File "/Users/talbpw/anaconda3/envs/raven_libraries/lib/python3.7/site-packages/pyomo/core/base/block.py", line 541, in __setattr__
            self.add_component(name, val)
        TypeError: _cashflow_rule() takes 3 positional arguments but 4 were given

... which is strange because it definitely takes 3 arguments and I'm not sure what the 4th is. Is it trying to pass t maybe?

Objective construction:

  def _create_objective(self, meta, m):
    rule = partial(self._cashflow_rule, meta)
    m.obj = pyo.Objective(rule=rule, sense=pyo.maximize)

Cashflow Rule

  def _cashflow_rule(self, meta, m):
    activity = m.Activity
    state_args = {'valued': False}
    total = self._compute_cashflows(m.Components, activity, m.Times, meta,
                                    state_args=state_args, time_offset=m.time_offset)
    return total

@dylanjm
Copy link
Collaborator

dylanjm commented Jan 20, 2022

@PaulTalbot-INL could it have something to do with partial() not knowing to skip the self parameter? I'm not certain this is the case but it appears there is a partialmethod() function that might fix that?

EDIT: This SO answer might explain more what I'm talking about: https://stackoverflow.com/questions/16626789/functools-partial-on-class-method

@PaulTalbot-INL
Copy link
Collaborator Author

PaulTalbot-INL commented Jan 20, 2022

Hm. partialmethod caused a new crash, saying from Pyomo's side that the object can't be used as a rule because it's an unknown type.

(1) Why do none of the constraints have this problem?
(2) Why is this problem new in pyomo 6.1?

Trying some things. Good thought, though.

The error is:

(etc)
          File "/Users/talbpw/projects/HERON/src/dispatch/pyomo_dispatch.py", line 659, in _create_objective
            m.obj = pyo.Objective(rule=rule, sense=pyo.maximize)
          File "/Users/talbpw/anaconda3/envs/raven_libraries/lib/python3.7/site-packages/pyomo/core/base/block.py", line 541, in __setattr__
            self.add_component(name, val)
        TypeError: Cannot treat the value 'functools.partialmethod(<bound method Pyomo._cashflow_rule of
 <dispatch.pyomo_dispatch.Pyomo object at 0x7fd7abe3f910>>, {'HERON': {'Case': <HERON Case>, 'Components': 
[<HERON Component "steamer"">, <HERON Component "generator"">, <HERON Component "electr_market"">, 
<HERON Component "electr_flex"">], 'Sources': [<HERON ARMA>, <HERON Function>], 'RAVEN_vars_full': {'Signal': 
<etc ...>)' 
as a constant because it has unknown type 'partialmethod'

@PaulTalbot-INL
Copy link
Collaborator Author

Ah, also, the SO you shared suggests the method isn't getting enough arguments, while in our case it says it's getting too many arguments.

@dylanjm
Copy link
Collaborator

dylanjm commented Jan 20, 2022

@PaulTalbot-INL I might have found the problem. Look at this line of code here in pyomo:

https://github.com/Pyomo/pyomo/blob/0edd35481feb2aa9d1f4e46ea231e84462fede42/pyomo/core/base/initializer.py#L112

if type(init) is functools.partial:
        _args = inspect.getfullargspec(init.func)
        if len(_args.args) - len(init.args) == 1 and _args.varargs is None:
            return ScalarCallInitializer(init)
        else:
            return IndexedCallInitializer(init)

I did some playing around and when we try to apply partial to class methods with a function that has > 2 parameters we will fail the first conditional and be returned as an IndexedCallInitializer. This class definition defines a __call__ method that tries to pass in the parent and the time index. which results in an extra parameter being passed into the function.

@PaulTalbot-INL
Copy link
Collaborator Author

This is interesting. Let's talk and see if we can convince it to behave like we want.

@dylanjm
Copy link
Collaborator

dylanjm commented Jan 20, 2022

I think I found the descrepency between 6.0 and 6.1. It can be found in the construct() method of the Objective class:

6.1
https://github.com/Pyomo/pyomo/blob/32a35a28e0c300dc89230dadbb46166cd649bba8/pyomo/core/base/objective.py#L283

6.0
https://github.com/Pyomo/pyomo/blob/be82bdcc7e8f38aaef2405c451bf3876763ab85f/pyomo/core/base/objective.py#L319

With the introduction of IndexedCallInitializer it sort of hardcodes the rule func to take an index variable even if we don't want it.

@dylanjm
Copy link
Collaborator

dylanjm commented Jan 27, 2022

@PaulTalbot-INL I think you may have accidently committed and pushed some scratch code. Let me know if not.

@PaulTalbot-INL
Copy link
Collaborator Author

yeah, this is WIP figuring out that silly change from 6.0 to 6.1. Not finished yet.

@PaulTalbot-INL PaulTalbot-INL marked this pull request as draft January 27, 2022 16:47
@dgarrett622
Copy link
Collaborator

I don't know why this makes a difference, but it does. From what I understand, functools.partial and lambda functions are very similar and for our application should be equivalent. I swapped out partial for lambda functions and all of the test cases run and mostly (cough Windows cough) pass using Pyomo 6.4.0.

I don't know if I can put my changes into this PR, but I can put up a new PR tomorrow for y'all to review.

@dgarrett622 dgarrett622 mentioned this pull request Apr 1, 2022
9 tasks
@PaulTalbot-INL
Copy link
Collaborator Author

superseded by #142.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants