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

Tests are failing when openpyxl is not installed. #568

Open
harshpalan opened this issue Dec 1, 2022 · 2 comments · May be fixed by #569
Open

Tests are failing when openpyxl is not installed. #568

harshpalan opened this issue Dec 1, 2022 · 2 comments · May be fixed by #569
Labels
bug Something isn't working tests changes to tests

Comments

@harshpalan
Copy link
Collaborator

harshpalan commented Dec 1, 2022

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?

@harshpalan harshpalan added bug Something isn't working tests changes to tests labels Dec 1, 2022
@genisplaja
Copy link
Collaborator

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
Copy link
Collaborator

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.

@magdalenafuentes magdalenafuentes linked a pull request Jan 27, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working tests changes to tests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants