-
Notifications
You must be signed in to change notification settings - Fork 192
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
Fix #5030 - C++ CLI: run.log is huge and missing the runner.registerWarning messages #5035
Conversation
if not is_labs: | ||
pytest.skip("classic (Ruby) CLI just ignored the top level --verbose flag for run.log") | ||
assert 50 < len(run_log.splitlines()) < 100 |
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.
If you did openstudio --verbose run -w workflow.osw
, would you expect the run.log
to contain Debug log messages?
As a reminder, we have ALL these ways of callling the CLI
openstudio --verbose ...
openstudio run --debug ...
openstudio run -w workflow.osw with workflow.osw having "run_options" > debug in there
# Labs only
openstudio --loglevel Debug ...
Follow up question: knowing that the Ruby CLI just didn’t set the run.log to Debug in that case (it only cares about run --debug ), would you:
- A) Use the historical behavior.
- B) Change it to respect the top level log level flag.
- if
--debug
is passed, it will increase the log level toDebug
if not already, but foropenstudio --loglevel Trace run --debug -w in.osw
, the Log Level for stdout and run.log would stay atTrace
)
- if
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.
If this is going to be merged into our imminent release, then I think we need to take option A and mirror the historical behavior (plus now the --loglevel option).
However, I think post release (and early in the cycle) we should make a change to remove the logging option under the run subcommand.
/** Whether registerInfo, registerWarning, registerError, registerInitialCondition, registerFinalCondition and registerAsNotApplicable | ||
* also LOG an actual message */ | ||
bool registerMsgAlsoLogs() const; | ||
void setRegisterMsgAlsoLogs(bool registerMsgAlsoLogs); |
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.
Here
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 feel like option like this is just punting on making a decision. I think I would prefer that we just make a call one way or the other and avoid giving an option like this.
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.
Until we remove the Ruby CLI (classic), we need an option, otherwise if always enabled, it'll log TWICE int he ruby cli
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.
@@ -78,6 +80,8 @@ OSWorkflow::OSWorkflow(const WorkflowRunOptions& t_workflowRunOptions, ScriptEng | |||
m_add_timings(t_workflowRunOptions.add_timings), | |||
m_style_stdout(t_workflowRunOptions.style_stdout) { | |||
|
|||
runner.setRegisterMsgAlsoLogs(true); |
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.
…kflow-gem) is really needed.
…rkflow_runner.rb used to do
…ow log level ``` openstudio --verbose openstudio --loglevel Debug openstudio run --debug ... openstudio run -w workflow.osw with workflow.osw having “run_options” > debug in there ```
… consideration for run.log
Address some of @shorowit's reports: * Remove debug print for numArs * stdout-energyplus is a one-liner on windows * EnergyPlus <status> is printed twice (one by E+, one by us after parsing), so alter message
…`No valid weather file defined in either the osm or osw.` > The old CLI always used to print warnings (e.g., `[17:21:45.141997 WARN] No valid weather file defined in either the osm or osw.`) to stdout, though I don't know if that's important. Just pointing it out as a difference. Some users might expect that behavior? Reported by @shorowit
…amples is installed, and I don't want these tests
…code=1 @shorowit reported: > One of our unit tests is failing because the OS CLI is returning an exit code of 0 instead of 1 for a case where a reporting measure returns false.
577789d
to
e13c0f6
Compare
CI Results for 87db4fb:
|
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 seems like this need to merge before release, but after release I think we should gather togethor and talk about the big picture of logging. Perhaps a wiki document or something to outline the design of logging in OpenStudio CLI and Workflow would be helpful. It is getting hard for me to track the options and I'm guessing I'm not the only one.
yeah, it would also be good to do a quick study on the performance impact of logging. I've seen performance go to sh*t when there is too much logging going on, espically with runner.registerINFO etc |
Pull request overview
Pull Request Author
src/model/test
)src/energyplus/Test
)src/osversion/VersionTranslator.cpp
)Labels:
IDDChange
APIChange
Pull Request - Ready for CI
so that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.