Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Improvement] Dependencies rationalization #58

Merged
merged 39 commits into from
Jul 2, 2021

Conversation

hadware
Copy link
Contributor

@hadware hadware commented Jun 23, 2021

A couple of updates:

  • Made the matplotlib dependency optional

  • Made pandas only required for testing/documentation (as it's only imported for typing purposes in the lib)

  • Made all additional and unnecessary dependencies installable via extra_requires, e.g.:

    pip install pyannote.core[notebook]
    pip install pyannote.core[doc]
    pip install pyannote.core[testing]
  • reflected these updates in the github actions configs.

  • added some documentation in the "Visualization" section

I'm sorry for the gory PR commit history... i don't know what i did wrong to have all these old commit listed in the PR.

hadware and others added 30 commits December 9, 2019 04:03
…ype hints should be good for the API objects.
# Conflicts:
#	pyannote/core/annotation.py
#	pyannote/core/segment.py
#	pyannote/core/timeline.py
…ailing in timeline.py. Added a type for LabelGenerator modes.
# Conflicts:
#	pyannote/core/segment.py
… couple of issues with the docstrings. Made use of "subset" for the Annotation's overlaps.
hadware and others added 4 commits June 23, 2021 02:40
Made the installation of matplotlib optional via the [notebook] extra require. Removed pandas dependency (as it is only used for input data, we can assume it has already been installed by the user code).
For notebook, testing and documentation, all dependencies can be installed via extra_requires in setup.py. Updated the github configs accordingly. Added page for objects vizualization.
pyannote/core/timeline.py Outdated Show resolved Hide resolved
@hbredin
Copy link
Member

hbredin commented Jun 23, 2021

Thanks for this PR -- this really makes things cleaner.
What do you thing about warning the users about the existence of pyannote.core[notebook] on ImportError?

@hadware
Copy link
Contributor Author

hadware commented Jun 25, 2021

What do you thing about warning the users about the existence of pyannote.core[notebook] on ImportError?

I added some warnings, tell me if this fits the bill

@hbredin
Copy link
Member

hbredin commented Jun 28, 2021

Thanks.

Sorry for nitpicking here but how can we be sure the ImportError is because of matplotlib and not another missing package?

Could we do something slightly smarter and try to import matplotlib at the top of pyannote.core.notebook and set a new variable MATPLOTLIB_IS_AVAILABLE to True and False accordingly?

@hadware
Copy link
Contributor Author

hadware commented Jun 28, 2021

This is a reasonable nitpick :)

However, my rationale was that, in this try/except statement, the only code that is imported (as part of the png rendering) is precisely matplotlib. If there's (for some very odd reason) an import error among matplotlib's dependencies that gets wrongly caught by the except ImportError block, this would still be the case with a "safe import" (as suggested) at the module level.

This would however be a good solution if you're planning on adding other visualization backends in the future.

@hbredin
Copy link
Member

hbredin commented Jun 28, 2021

This would however be a good solution if you're planning on adding other visualization backends in the future.

That would have been my reply before you edited your answer :)
So yes, can you please do the change to be future-proof?

@hadware
Copy link
Contributor Author

hadware commented Jul 1, 2021

I made the warnings flag-enabled, is this how you expected them to be?

from .notebook import repr_feature
from .notebook import MATPLOTLIB_IS_AVAILABLE, MATPLOTLIB_WARNING
if not MATPLOTLIB_IS_AVAILABLE:
warnings.warn(MATPLOTLIB_WARNING.format(obj=self))
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't obj be the class name rather than the instance?

>>> MATPLOTLIB_WARNING.format(obj=Annotation())
Couldn't import matplotlib to render the vizualization for object . To enable, install the required dependencies with 'pip install pyannore.core[notebook]'

>>> MATPLOTLIB_WARNING.format(obj=Annotation().__class__.__name__)
Couldn't import matplotlib to render the vizualization for object Annotation. To enable, install the required dependencies with 'pip install pyannore.core[notebook]'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, sorry for that.

@hbredin hbredin merged commit f2f131d into pyannote:develop Jul 2, 2021
@hbredin
Copy link
Member

hbredin commented Jul 2, 2021

Will make a proper release when PR for #57 is also merged.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants