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

Fix crash #323

Merged
merged 2 commits into from
Aug 7, 2017
Merged

Fix crash #323

merged 2 commits into from
Aug 7, 2017

Conversation

vext01
Copy link
Member

@vext01 vext01 commented Aug 7, 2017

This fixes the crash we saw yesterday:

Message from krun running on bencher5:

Fatal Krun error: 'NoneType' object has no attribute 'eta_estimates'
  File "krun/krun.py", line 231, in main
    inner_main(mailer, on_first_invocation, config, args)
  File "krun/krun.py", line 357, in inner_main
    sched.run()
  File "/home/kruninit/warmup_experiment/krun/krun/scheduler.py", line 531, in run
    extra_env=self._make_post_cmd_env(results)
  File "/home/kruninit/warmup_experiment/krun/krun/scheduler.py", line 473, in _make_post_cmd_env
    eta_val = self.get_overall_time_estimate_formatter(results).finish_str
  File "/home/kruninit/warmup_experiment/krun/krun/scheduler.py", line 462, in get_overall_time_estimate_formatter
    self.get_estimated_overall_duration(results))
  File "/home/kruninit/warmup_experiment/krun/krun/scheduler.py", line 447, in get_estimated_overall_duration
    per_exec = self.get_estimated_exec_duration(key, results)
  File "/home/kruninit/warmup_experiment/krun/krun/scheduler.py", line 438, in get_estimated_exec_duration
    previous_exec_times = results.eta_estimates.get(key)

I'm not sure why my testing didn't find this when I was implementing the re-run logic, but anyway, I've re-tested carefully and I think this is good.

The first commit is the fix, the message explains what went wrong before.

The second commit is just some defensive programming.

OK?

Fixes a crash with running post-exec commands for a re-run.
_make_post_cmd_env() needs results to put the ETA in the env. Before the
results were 'None'.
@ltratt
Copy link
Member

ltratt commented Aug 7, 2017

I'm fine with this except assert False is an icky idiom IMHO (asserts are removed by -O for example): we should do something like raise RuntimeError.

@vext01
Copy link
Member Author

vext01 commented Aug 7, 2017

I didn't know about -O (but it's quite amusing somehow in the case of CPython).

How about a call to fatal instead?

@ltratt
Copy link
Member

ltratt commented Aug 7, 2017

I'd keep it simple: since it's not that likely to happen, raise RuntimeError or equivalent seems easy enough.

@vext01
Copy link
Member Author

vext01 commented Aug 7, 2017

Here we go. See also #324 as there are quite a few assertions in-tree (I don't want to fix those immediately though).

@ltratt
Copy link
Member

ltratt commented Aug 7, 2017

OK, squash and I'll merge.

@vext01
Copy link
Member Author

vext01 commented Aug 7, 2017

splat.

@vext01
Copy link
Member Author

vext01 commented Aug 7, 2017

Fixed typo.

@ltratt ltratt merged commit 9c38f01 into master Aug 7, 2017
@ltratt ltratt deleted the fix-crash branch August 7, 2017 10:47
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.

2 participants