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

Fix continuous integration #672

Merged
merged 11 commits into from
Jul 6, 2023
Merged

Conversation

LeonieBorne
Copy link
Contributor

@LeonieBorne LeonieBorne commented Jul 5, 2023

Hi @mpariente 🐿️

I tried to fix the continuous integration before doing a new release (as mentioned in #664).
It should work 🤞 But I have a couple of questions:

  • the commits I made from my previous pull request are also present in this pull request and I am not sure why. It actually created conflicts and I had to merge master into ci_fix branch. Should I have forked the repo a second time before doing my new commits? Or rebase my development branch as proposed here?
  • scipy.signal.get_window(window="hanning") is not supported in newer version of scipy and is replaced by window="hann" (check documentation v0.11.0 vs v1.11.1). I didn't succeed to install a version of scipy which works with window="hanning" and is compatible with the others libraries installed in the requirements. So instead, I replaced hanning by hann in the arguments and changed the requirements to scipy>=1.10.1 (which works on my computer). I feel like it is not the best solution as window="hanning" was working before, should I change this fix?

Thanks!

@LeonieBorne LeonieBorne closed this Jul 5, 2023
@LeonieBorne LeonieBorne reopened this Jul 5, 2023
@mpariente
Copy link
Collaborator

This is perfect, thank you ! ✨

It's in the commits, but I rewrite it here for more visibility : pytorch-lightning was limited to 1.7.7 because the Callback.on_epoch_start hook was removed in v1.8.

@mpariente mpariente merged commit fd6a963 into asteroid-team:master Jul 6, 2023
@mpariente
Copy link
Collaborator

Regarding your questions :

  • You could have waited for the PR to be merged, update your fork using upstream, and create a new branch from there. But doing it how you did is fine, because we squash the commits when merging, so the git history is not polluted.
  • It's OK to assume latest versions of common librairies.

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

Successfully merging this pull request may close these issues.

2 participants