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 kaleido and plotly versions #163

Merged
merged 3 commits into from Aug 30, 2021

Conversation

mmeendez8
Copy link
Contributor

Fixes kaleido permission err:

PermissionError: [Errno 13] Permission denied: '/home/mmendez/anaconda3/envs/pyodi/lib/python3.7/site-packages/kaleido-0.1.0-py3.7-linux-x86_64.egg/kaleido/executable/kaleido'

@mmeendez8 mmeendez8 added the bug Something isn't working label Aug 27, 2021
@mmeendez8 mmeendez8 requested a review from daavoo August 27, 2021 16:26
@mmeendez8 mmeendez8 changed the title Update versions Update kaleido and plotly versions Aug 27, 2021
@daavoo
Copy link
Collaborator

daavoo commented Aug 27, 2021

Why wasn't this catched by C.I.??

@mmeendez8
Copy link
Contributor Author

mmeendez8 commented Aug 27, 2021

Seems to only prompt when trying to save to a local file.

Adding a simple test now for avoiding future problems

from pyodi.core.boxes import add_centroids
from pyodi.core.utils import coco_ground_truth_to_df
from pyodi.plots.boxes import get_centroids_heatmap, plot_heatmap
from pyodi.plots.common import plot_scatter_with_histograms


@logger.catch
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@daavoo this was catching all exceptions without reraising anything... so if something crashes, tests simply skips it. Can't recall why did we add this

Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be: logger.catch(reraise=True)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should remove all those decorator.

This is useful to ensure unexpected exceptions are logged, the entire program can be wrapped by this method
https://loguru.readthedocs.io/en/stable/api/logger.html#loguru._logger.Logger.catch

I am not seeing any advantage on using this here

Copy link
Collaborator

@daavoo daavoo Aug 30, 2021

Choose a reason for hiding this comment

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

As you wish. With reraise=True the exceptions would work as without the decorator. In the past, I had found the additional information showed by loguru useful for debugging exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'll remove all those decorators in a different issue, let's merge this for now

@mmeendez8 mmeendez8 merged commit aa6974a into master Aug 30, 2021
@mmeendez8 mmeendez8 deleted the update-plotly-and-kaleido-versions branch August 30, 2021 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

None yet

2 participants