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

BUG: MemorizedFunc.call output changed between 1.3.2 and 1.4.0 #1572

Open
larsoner opened this issue Apr 16, 2024 · 3 comments
Open

BUG: MemorizedFunc.call output changed between 1.3.2 and 1.4.0 #1572

larsoner opened this issue Apr 16, 2024 · 3 comments

Comments

@larsoner
Copy link
Contributor

The short version is that the metadata is no longer returned after #894. You can see it in the docstring of the Returns section of MemorizedFunc.call in the diff here:

https://github.com/joblib/joblib/pull/894/files#diff-892315c4227d75d32ad4675e8aaee76fb62a998e9e609cc204df1dbc4272f84bL849-L850

Was this intentional? It broke some downstream code, e.g.:

https://github.com/mne-tools/mne-bids-pipeline/blob/cbeeb98d0625ac948f8a6951305de11520eac5d5/mne_bids_pipeline/_run.py#L280

https://app.circleci.com/pipelines/github/mne-tools/mne-bids-pipeline/4478/workflows/c9b0bd11-4a7b-4193-aeeb-683979144ef0/jobs/64079

If so maybe the note that says Allow caching co-routines with Memory.cache. in the release log could be updated with this backward incompatible change info?

@tomMoral
Copy link
Contributor

Hello,
Thanks for the report and sorry for the incompatible change. This was not intentional, it came in a large refactor and apparently we had not regression test for this API.

I am not sure what is the best course of action now. I think the API would be cleaner by not returning the metadata as this allows to call either the function of call the same way.
But this means that potentially a lot of code will need to support both or force updates, which is quite troublesome.
I think for the sake of maintainability of downstream packages, I would go for a BF release but happy to take an opinion here (@ogrisel @GaelVaroquaux )

@larsoner
Copy link
Contributor Author

FWIW it wasn't too difficult in our case to put a simple workaround in place so for us it's no longer a problem. So I guess it's a question of how extensively the API is used by others.

@tomMoral
Copy link
Contributor

I think for the purpose of consistent API, we should make a BF release.
If we change something, we should do it explicitly, not by mistake.. 😓

Thanks for the report, I will revert this change and add some non-regression tests.

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

No branches or pull requests

2 participants