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

Update PyROS Solver Logging System #2990

Merged
merged 85 commits into from
Nov 8, 2023

Conversation

shermanjasonaf
Copy link
Contributor

@shermanjasonaf shermanjasonaf commented Sep 9, 2023

Summary/Motivation:

There are a few areas of improvement for the current PyROS solver logging system:

  • The level of the progress_logger argument to the PyROS solver is forced to logging.INFO.
    This limits the extent to which the user can control the logging output. Moreover, the original level of progress_logger is not restored at termination.
  • Introductory information is currently displayed with print() statements.
  • The iteration logs display only the iteration number. We wish to provide additional
    information about the iterates.
  • A termination message is not logged if there is a subsolver error and the user does not specify
    an intent to have subproblems serialized.

Additionally, in the event PyROS terminates with a max_iter status, then the number of iterations is currently overcounted by 1.

Changes proposed in this PR:

  • Afford user greater control of the progress logger. In particular:
    • If the user provides a customized progress logger, don't force the logger to INFO level. Otherwise, document any modifications made.
    • Use the various standard Python logging levels for different levels of detail.
  • Update the default PyROS solver logger object.
  • Make solver logs more detailed, i.e. provide or enhance:
    • ERROR-level messages in advance of exceptions raised by subsolvers or the coefficient matching routine.

    • WARNING-level messages on nonacceptable subproblem terminations, backup solver invocation, and bypassing of global separation

    • INFO-level messages for:

      • Detailed solver version information (including Git branch/commit info for developers)
      • summary of user options.
      • breakdown of model component statistics.
      • iteration logs which summarize the PyROS master and separation subproblem outcomes, and serve to inform
        the user on the progress of the algorithm
      • summary of PyROS termination (solve time, iterations, final objective, termination condition, solve time breakdown obtained through HierarchicalTimer)
    • DEBUG-level messages on individual subproblem outcomes, and the realizations added to the master problem.

  • Add a section on the logging system to the online docs.
  • Add tests for various components of the PyROS solver output log.
  • Fix iteration overcounting for the case in which PyROS terminates with a max_iter status.
  • Standardize ROSolveResults setup at solver termination

TODOs

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.

Copy link
Contributor

@natalieisenberg natalieisenberg left a comment

Choose a reason for hiding this comment

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

Look good to me! I like the improvements to logging.

@blnicho blnicho requested a review from jsiirola October 3, 2023 18:53
@codecov
Copy link

codecov bot commented Oct 4, 2023

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (00e9324) 87.87% compared to head (3e9f156) 87.94%.
Report is 32 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2990      +/-   ##
==========================================
+ Coverage   87.87%   87.94%   +0.07%     
==========================================
  Files         769      769              
  Lines       89522    89807     +285     
==========================================
+ Hits        78668    78982     +314     
+ Misses      10854    10825      -29     
Flag Coverage Δ
linux 85.24% <85.82%> (+0.04%) ⬆️
osx 75.07% <85.57%> (+0.06%) ⬆️
other 85.42% <85.82%> (+0.03%) ⬆️
win 82.48% <85.82%> (+0.05%) ⬆️

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

Files Coverage Δ
pyomo/contrib/pyros/solve_data.py 94.40% <95.83%> (+1.21%) ⬆️
pyomo/contrib/pyros/pyros.py 95.72% <97.29%> (+0.58%) ⬆️
pyomo/contrib/pyros/pyros_algorithm_methods.py 96.09% <98.00%> (+2.60%) ⬆️
pyomo/contrib/pyros/util.py 89.49% <98.05%> (+3.63%) ⬆️
pyomo/contrib/pyros/master_problem_methods.py 89.40% <88.67%> (+1.05%) ⬆️
pyomo/contrib/pyros/separation_problem_methods.py 94.11% <85.41%> (-0.62%) ⬇️

... and 5 files with indirect coverage changes

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

Copy link
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.

Looks good. Some comments, but nothing to prevent merging

pyomo/contrib/pyros/pyros.py Outdated Show resolved Hide resolved
pyomo/contrib/pyros/pyros.py Outdated Show resolved Hide resolved
pyomo/contrib/pyros/pyros_algorithm_methods.py Outdated Show resolved Hide resolved
Copy link
Contributor

@mrmundt mrmundt left a comment

Choose a reason for hiding this comment

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

A general comment that does not need to be addressed in this PR, but if PyROS is ever graduated from contrib to core, I will strongly request the change from master to more inclusive language (e.g., primary).

Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to be an inclusion suggestion: is it possible to change references of master to something like primary? master is a prime example of noninclusive language.

@mrmundt mrmundt merged commit 670d8a1 into Pyomo:main Nov 8, 2023
27 checks passed
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.

6 participants