Skip to content

Update kaleido and plotly versions#163

Merged
mmeendez8 merged 3 commits into
masterfrom
update-plotly-and-kaleido-versions
Aug 30, 2021
Merged

Update kaleido and plotly versions#163
mmeendez8 merged 3 commits into
masterfrom
update-plotly-and-kaleido-versions

Conversation

@mmeendez8
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

daavoo commented Aug 27, 2021

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

@mmeendez8
Copy link
Copy Markdown
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.plots.common import plot_scatter_with_histograms


@logger.catch
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

2 participants