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.

Sign up for GitHub

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

Jump to bottom

Log traceback in case of exceptions during optimizations #1156

Merged
merged 8 commits into from
Oct 31, 2023
Merged

Log traceback in case of exceptions during optimizations #1156

merged 8 commits into from
Oct 31, 2023

Conversation

dweindl
Copy link
Member

@dweindl dweindl commented Oct 25, 2023 •
edited
Loading

So far, with minimize(..., OptimizeOptions(allow_failed_starts=True))
only the exception message was stored/logged, which is not always sufficiently
informative (see #1147).

Now we log the full exception type / message / traceback.

Closes #1147

@dweindl
More informative OptimizerResult.message in case of exceptions
72e0e7b 
So far only the exception message was stored if an exception occurred
during optimization with FidesOptimizer. This is often enough not
helpful at all.
Now this includes exception type / message / traceback.

Related to #1147
PaulJonasJost
PaulJonasJost approved these changes Oct 25, 2023
View reviewed changes
Copy link
Collaborator

@PaulJonasJost PaulJonasJost left a comment

Choose a reason for hiding this comment

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

Does this include the whole traceback now? might be quite long, but good to have for this anyways.

Not sure on policies we have on importing, but possibly nicer to have them as default import for the sake of better overview?

@dweindl
Copy link
Member Author

dweindl commented Oct 25, 2023

Does this include the whole traceback now? might be quite long, but good to have for this anyways.

Yes. One can always shorten it later. But previously this didn't contain sufficient information to debug anything.

Not sure on policies we have on importing, but possibly nicer to have them as default import for the sake of better overview?

No clear policy. But if those modules are only required under exceptional circumstances, i would only import them under exceptional circumstances.
I would rather not go back to #934

@codecov-commenter
Copy link

codecov-commenter commented Oct 25, 2023 •
edited
Loading

Codecov Report

Merging #1156 (80be349) into develop (160c2a8) will decrease coverage by 3.84%.
Report is 409 commits behind head on develop.
The diff coverage is 87.81%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@             Coverage Diff             @@
##           develop    #1156      +/-   ##
===========================================
- Coverage    88.16%   84.33%   -3.84%     
===========================================
  Files           79      148      +69     
  Lines         5257    11683    +6426     
===========================================
+ Hits          4635     9853    +5218     
- Misses         622     1830    +1208
Files Coverage Δ
pypesto/C.py 100.00% <100.00%> (ø)
pypesto/__init__.py 100.00% <100.00%> (ø)
pypesto/engine/__init__.py 100.00% <100.00%> (ø)
pypesto/engine/multi_process.py 93.10% <100.00%> (+0.79%) ⬆️
pypesto/engine/multi_thread.py 100.00% <100.00%> (ø)
pypesto/engine/single_core.py 100.00% <100.00%> (ø)
pypesto/engine/task.py 100.00% <100.00%> (ø)
pypesto/ensemble/__init__.py 100.00% <100.00%> (ø)
pypesto/ensemble/task.py 100.00% <100.00%> (ø)
pypesto/hierarchical/__init__.py 100.00% <100.00%> (ø)
... and 138 more

@dweindl dweindl marked this pull request as draft October 25, 2023 13:55
@dweindl
Copy link
Member Author

dweindl commented Oct 25, 2023

Okay, this is not quite what we want. This messes up handling of cases like RuntimeError: Encountered non-finite function inf value at initial point..

@dweindl
Revert "More informative OptimizerResult.message in case of excepti…
bb971a9 
…ons"

This reverts commit 72e0e7b.
@dweindl
Log traceback in case of exceptions during optimizations
b90b6e0 
So far, with `minimize(..., OptimizeOptions(allow_failed_starts=True))`
only the exception message was stored/logged, which is not always sufficiently
informative (see #1147).

Now we log the full exception type / message / traceback.

Closes #1147
@dweindl dweindl changed the title More informative OptimizerResult.message in case of exceptions Log traceback in case of exceptions during optimizations Oct 30, 2023
@dweindl dweindl marked this pull request as ready for review October 30, 2023 10:19
@dweindl dweindl self-assigned this Oct 30, 2023
@dweindl dweindl linked an issue Oct 31, 2023 that may be closed by this pull request
Uninformative 'hess' in OptimizerResult.message #1147
Closed
@dweindl
Copy link
Member Author

dweindl commented Oct 31, 2023

Okay, this is not quite what we want. This messes up handling of cases like RuntimeError: Encountered non-finite function inf value at initial point..

Changed now. Leaving FidesOptimizer exception reporting as it was, just changing it for the non-caught exceptions. Only logging the full traceback, but keeping the result message short. Should be reasonable compromise.

@PaulJonasJost Please re-review.

PaulJonasJost
PaulJonasJost approved these changes Oct 31, 2023
View reviewed changes
Copy link
Collaborator

@PaulJonasJost PaulJonasJost left a comment

Choose a reason for hiding this comment

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

Thanks, agreed that this seems like a reasonable compromise

@dweindl dweindl merged commit b0b39f0 into develop Oct 31, 2023
18 checks passed
@dweindl dweindl deleted the fides_exception_msg branch October 31, 2023 14:43
This was referenced Nov 15, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PaulJonasJost PaulJonasJost PaulJonasJost approved these changes

Assignees

@dweindl dweindl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Uninformative 'hess' in OptimizerResult.message
3 participants
@dweindl @codecov-commenter @PaulJonasJost