#580 python39 and remove python3.6 #586
Conversation
Codecov Report
@@ Coverage Diff @@
## master #586 +/- ##
==========================================
+ Coverage 96.96% 96.99% +0.02%
==========================================
Files 58 58
Lines 6990 6987 -3
==========================================
- Hits 6778 6777 -1
+ Misses 212 210 -2 |
| # for local mp3s only | ||
| except TypeError: | ||
| duration = librosa.get_duration(filename=audio_path) | ||
| duration = librosa.get_duration(path=audio_path) |
There was a problem hiding this comment.
@genisplaja I'm not sure what's the motivation behind reading the audio using with open() to get the duration later rather than using librosa.load... Also, not sure why now it's not passing codecov and previously it was.
There was a problem hiding this comment.
How is that standing now? Should we still take a look?
There was a problem hiding this comment.
this is ok, now. I don't know what happened the other time I tried, but seems ok now. Actually it is a future deprecation warning from librosa, so good to incorporate that in this PR
There was a problem hiding this comment.
Thanks @guillemcortes, looks good :) Just left a couple of comments (actually more questions than comments). Just a couple of minor things:
- Should we mention somewhere that we discontinue the support to Python3.6 but that if someone is interested on using that they can get mirdata<=0.3.6 (or 0.3.7 actually) which is the last one compatible with Python3.6?
- Probably that fact that the 3.6 test is still there is because the circleci config right? Maybe we could merge and see what happens in the other open PR?
| @@ -1,6 +1,7 @@ | |||
| # mirdata | |||
| common loaders for Music Information Retrieval (MIR) datasets. Find the API documentation [here](https://mirdata.readthedocs.io/). | |||
|
|
|||
| [](https://www.python.org/) | |||
| and install all ``mirdata`` requirements. You will want to install the latest versions of Python 3.6 and 3.7. | ||
| Once ``pyenv`` and the Python versions are configured, install ``pytest``. Make sure you installed all the pytest | ||
| plugins to automatically test your code successfully. Finally, run: | ||
| and install all ``mirdata`` requirements. Once ``pyenv`` and the Python versions are configured, install ``pytest``. |
There was a problem hiding this comment.
are we skipping the sentences "you will want to install the latests version of Python 3.7 and 3.8"?
There was a problem hiding this comment.
In my opinion is something redundant and requires changes every time we support a new python version.
There was a problem hiding this comment.
maybe we could generalize it to "you will want to install the latest versions of Python"... if we want it to be here of course.
| # for local mp3s only | ||
| except TypeError: | ||
| duration = librosa.get_duration(filename=audio_path) | ||
| duration = librosa.get_duration(path=audio_path) |
There was a problem hiding this comment.
How is that standing now? Should we still take a look?
I think we can include this into the next release notes. If this is too hidden, we can maybe add a changelog.md file with the info for easy tracking?
yeah, let's see if @magdalenafuentes got more info, but yeah, it is strange behavior EDIT: I see that at soundata there's a changelog.rst > https://github.com/soundata/soundata/blob/main/docs/source/changelog.rst that points to github release notes. We could just do the same, if you agree. |
guillemcortes
added
duplicate
|
Closing the PR since this has ben already addressed in #596 |
Reference issue: #580