mir-dataset-loaders / mirdata

Python library for working with Music Information Retrieval datasets
https://mirdata.readthedocs.io/en/stable/
BSD 3-Clause "New" or "Revised" License
357 stars 59 forks source link

Tests are failing when openpyxl is not installed. #568

Open harshpalan opened 1 year ago

harshpalan commented 1 year ago

When running pytest tests\ --local after the 0.3.7 release, the tests are failing since they are not able to get openpyxl and it escaping the try/except block added in the dataset (compmusic_carnatic_rhythm and compmusic_hindustani_rhythm)

ImportError while importing test module 'E:\MARL\myrepo\mirdata\tests\datasets\test_carnatic_rhythm.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
c:\users\harsh\appdata\local\programs\python\python37\lib\importlib\__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
tests\datasets\test_carnatic_rhythm.py:4: in <module>
    from mirdata.datasets import compmusic_carnatic_rhythm
mirdata\datasets\compmusic_carnatic_rhythm.py:60: in <module>
    from openpyxl import load_workbook as get_xlxs
Hint: make sure your test modules/packages have valid Python names.
tests\datasets\test_hindustani_rhythm.py:4: in <module>
    from mirdata.datasets import compmusic_hindustani_rhythm
mirdata\datasets\compmusic_hindustani_rhythm.py:58: in <module>
    from openpyxl import load_workbook as get_xlxs
E   ModuleNotFoundError: No module named 'openpyxl'

I and @magdalenafuentes were thinking if we should add all dataset-dependent packages to tests in setup.py. @genisplaja and @nkundiushuti what do you guys think about this solution?

genisplaja commented 1 year ago

Hi both :) Ok! However there is something that I don't understand. Are compmusic_carnatic_rhythm and compmusic_hindustani_rhythm the only datasets for which the tests fail? Following this problem I think DALI and the other datasets that have optional dependencies should fail as well right? Just to confirm. I think adding all the optional packages in [test] installations is a good idea. Maybe it'd be a good chance to implement the new optional dependency strategy that @magdalenafuentes mentioned in #562?

magdalenafuentes commented 1 year ago

You're right, the only reason why they wouldn't break is because in the docs it is indicated to install those dependencies. We're removing those instructions and adding optional dependencies to the test dependencies, so it's smooth for the user.