Skip to content

Conversation

@vkarak
Copy link
Contributor

@vkarak vkarak commented Nov 29, 2022

This PR introduces the general/perf_info_level configuration parameter and the RFM_PERF_INFO_LEVEL environment variable that control the level at which the immediate performance info is printed (i.e., the P: lines after a performance test finishes). The default level is info, which causes the info to be printed always, but it can be set to verbose if users want this output to be printed only with the -v option.

Closes #2666.

Copy link
Contributor

@victorusu victorusu left a comment

Choose a reason for hiding this comment

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

I loved this feature! Thanks for this!
I am wondering here whether having a dedicated flag just for performance debugging would be better because the performance debugging increases also for everything else. This is a bit annoying when you are "just interested" in debugging the performance lines, which is in general my case...
Having said that, I agree that having a single cmd option for debugging is consistent and this is just another debugging compared to any other one... The main difference is that in this case, we have dedicated control...
What do you think?

@victorusu
Copy link
Contributor

I have to confess that my tests here are not that bad! I am so used to the debugging info that they actually do not fog my ability to focus on the performance one... 😄

@vkarak
Copy link
Contributor Author

vkarak commented Dec 9, 2022

Can you post an example of what is your case? This PR is just about which level the immediate performance info will be printed. If you set it to verbose, then this info is going to show up only when the -v option is passed. As described in the issue, it's useful to control whether you want that information or not: if you run interactively, then you need that information printed (without all the verbose messages), so in that case you set its level info to info (the default). If you are not running interactive and you want a cleaner output, you select to print this information only in verbose level. Both scenarios are valid.

@codecov-commenter
Copy link

codecov-commenter commented Dec 10, 2022

Codecov Report

Base: 86.61% // Head: 86.59% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (1f7f19f) compared to base (7b487ce).
Patch coverage: 91.66% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2695      +/-   ##
==========================================
- Coverage   86.61%   86.59%   -0.02%     
==========================================
  Files          60       60              
  Lines       11197    11198       +1     
==========================================
- Hits         9698     9697       -1     
- Misses       1499     1501       +2     
Impacted Files Coverage Δ
reframe/utility/typecheck.py 96.68% <50.00%> (-0.63%) ⬇️
reframe/core/logging.py 80.95% <100.00%> (+0.07%) ⬆️
reframe/frontend/argparse.py 91.72% <100.00%> (-1.03%) ⬇️
reframe/frontend/cli.py 73.84% <100.00%> (+0.04%) ⬆️
reframe/frontend/executors/policies.py 92.52% <100.00%> (+0.02%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@ekouts ekouts left a comment

Choose a reason for hiding this comment

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

Overall it looks good to me and I think it's on the right direction. I noticed that the error can be very misleading in case the log level has a typo:

$ export RFM_PERF_INFO_LEVEL=infus

$ ./bin/reframe ...

[----------] start processing checks
[ RUN      ] Foo /1cbec737 @generic:default+builtin
[       OK ] (1/1) Foo /1cbec737 @generic:default+builtin
[  PASSED  ] Ran 1/1 test case(s) from 1 check(s) (0 failure(s), 0 skipped)
[==========] Finished on Mon Dec 12 09:07:03 2022 
Run report saved in '/Users/ekoutsaniti/.reframe/reports/run-report-218.json'
ERROR: run session stopped: attribute error: 'AsynchronousExecutionPolicy' object has no attribute '_advance_retired'
ERROR: Traceback (most recent call last):
  File "/Users/ekoutsaniti/repos/reframe-fork/reframe/frontend/cli.py", line 1275, in main
    runner.runall(testcases, restored_cases)
  File "/Users/ekoutsaniti/repos/reframe-fork/reframe/core/logging.py", line 887, in _fn
    return fn(*args, **kwargs)
  File "/Users/ekoutsaniti/repos/reframe-fork/reframe/frontend/executors/__init__.py", line 516, in runall
    self._runall(testcases)
  File "/Users/ekoutsaniti/repos/reframe-fork/reframe/frontend/executors/__init__.py", line 577, in _runall
    self._policy.exit()
  File "/Users/ekoutsaniti/repos/reframe-fork/reframe/frontend/executors/policies.py", line 340, in exit
    self._advance_all(self._current_tasks, timeout)
  File "/Users/ekoutsaniti/repos/reframe-fork/reframe/frontend/executors/policies.py", line 411, in _advance_all
    bump_state = getattr(self, f'_advance_{t.state}')
AttributeError: 'AsynchronousExecutionPolicy' object has no attribute '_advance_retired'

Log file(s) saved in '/var/folders/fk/vjppffg16jd1yv63wfbgsjd40000gp/T/rfm-_znxnhb0.log'

I think we should check that the string can be translated to a log level before running the tests and issue at least a warning. What do you think?

@vkarak
Copy link
Contributor Author

vkarak commented Dec 12, 2022

I think we should check that the string can be translated to a log level before running the tests and issue at least a warning. What do you think?

In the config, I'm using an enum, but I'm not checking the value when it comes from the environment variable. I'll fix that.

@vkarak vkarak requested a review from ekouts December 13, 2022 20:34
@vkarak vkarak merged commit 750f7ea into reframe-hpc:master Dec 15, 2022
@vkarak vkarak deleted the feat/control-verbosity-perf-info branch December 15, 2022 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The immediate performance printout is too verbose

4 participants