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

Bump dependencies #99

Merged
merged 3 commits into from
Jul 7, 2021
Merged

Conversation

dhirschfeld
Copy link
Contributor

Resolves #98

@google-cla google-cla bot added the cla: yes label May 31, 2021
@dhirschfeld
Copy link
Contributor Author

Figured I'd see if it breaks anything! 😅

@dhirschfeld
Copy link
Contributor Author

ping! Anyone able to take a look? At least kick off the CI?

@lxuechen lxuechen self-requested a review June 4, 2021 20:43
setup.py Outdated
@@ -34,7 +34,7 @@
description="SDE solvers and stochastic adjoint sensitivity analysis in PyTorch.",
url="https://github.com/google-research/torchsde",
packages=setuptools.find_packages(exclude=['benchmarks', 'diagnostics', 'examples', 'tests']),
install_requires=['torch>=1.6.0', 'numpy==1.19.*', 'boltons>=20.2.1', 'trampoline>=0.1.2', 'scipy==1.5.*'],
install_requires=['torch>=1.6.0', 'numpy==1.20.*', 'boltons>=20.2.1', 'trampoline>=0.1.2', 'scipy==1.6.*'],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we change this to >=1.19.0 or something like that? Thanks.

Copy link
Collaborator

@lxuechen lxuechen Jun 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also scipy==1.5.* => 'scipy>=1.5.*'?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was conservative with the changes as some maintainers prefer to pin to the minor version so as to avoid potential future breakage when a new release comes out. That of course, has to be weighed against the overhead of having to manually update the deps.

At least with conda pins can be applied retrospectively if a new version of a dependency breaks things so it's a bit safer to pin loosely.

@lxuechen
Copy link
Collaborator

lxuechen commented Jun 13, 2021

The Py3.6 runs failed due to scipy not being compatible. I just replicated this behaviour on my machine as well. I think a simple workaround is to enforce the version more strictly using the Python version fetched during run time in setup.py. WDYT?

Happy to also accept other potentially better sol'ns. Thanks for the patience again.

@patrick-kidger
Copy link
Collaborator

@lxuechen One possible resolution -- I'd be happy to bump the minimum Python version to 3.7 if you are? I hear on the grapevine that that's the minimum version PyTorch is going to be supporting pretty soon anyway.

@dhirschfeld
Copy link
Contributor Author

I'd be happy to bump the minimum Python version to 3.7 if you are?

That works for me. The latest scipy already has a minimum version of py37:
scipy/scipy#13081

@lxuechen
Copy link
Collaborator

I'd be happy to bump the minimum Python version to 3.7 if you are?

That works for me. The latest scipy already has a minimum version of py37:
scipy/scipy#13081

For now, I think since torch hasn't dropped support for Python3.5, I'd prefer that we don't up the min version yet. Feel free to leave a TODO in setup.py so that we may revisit in the future.

@lxuechen
Copy link
Collaborator

Thanks again for sending in this PR, @dhirschfeld ! I still plan to merge this even though there's been a huge delay. If you could do a rebase from master and add the runtime Python version checks, I can merge this right away.

Again, sorry for the unresponsiveness on my end.

@dhirschfeld
Copy link
Contributor Author

Hi @lxuechen, sorry I've been meaning to circle back to this but I'm pretty smashed with the end of the fin-year coming up. I might not get around to revisiting for a couple of weeks so I'm happy for you to take this over or to start fresh if you's like it in sooner - whatever is easiest.

@Sayam753
Copy link

Sayam753 commented Jul 6, 2021

Hi @lxuechen . Is this PR ready to be merged? or are there some TODOs still remaining?

@Sayam753
Copy link

Sayam753 commented Jul 6, 2021

Here is the PR https://github.com/Sayam753/torchsde/pull/1 at my fork which bumps the dependencies. I can roll out a PR in this repo if this is the right way to update dependencies :)

@dhirschfeld
Copy link
Contributor Author

The last scipy compatible with py36 was 1.5.4:
https://github.com/scipy/scipy/blob/v1.5.4/setup.py#L539

Since scipy=1.7.0 isn't compatible with py36 pip should never have tried installing it. Unfortunately pip isn't smart enough to work that out :/

@dhirschfeld
Copy link
Contributor Author

@lxuechen / @patrick-kidger can you please approve the CI so that I can test out a solution using platform specific dependencies?

image

@patrick-kidger
Copy link
Collaborator

Approved the CI but I don't see any platform-specific dependencies in the current PR.
(Sorry that this has been such a faff to resolve, by the way.)

@dhirschfeld
Copy link
Contributor Author

I don't see any platform-specific dependencies in the current PR.

Right, sorry - didn't push. Looks like it needs to be approved on every commit? That's not a great UX/DX for anyone! 😬

@dhirschfeld
Copy link
Contributor Author

Same issue with numpy - 1.19.5 was the last version to support py36

@dhirschfeld
Copy link
Contributor Author

Sorry that this has been such a faff to resolve, by the way.

No worries - looks like we got there in the end! 🎉

@patrick-kidger patrick-kidger merged commit 17fc386 into google-research:master Jul 7, 2021
@patrick-kidger
Copy link
Collaborator

LGTM! Thanks for contributing, and resolving this issue.

@dhirschfeld dhirschfeld deleted the bump-deps branch July 7, 2021 11:43
@Sayam753
Copy link

Sayam753 commented Jul 7, 2021

Thanks for the fast response and merging in the PR :)

@dhirschfeld
Copy link
Contributor Author

Just wanted to check if a release was imminent? If not I might patch the deps on the existing conda package so that it's installable with the latest verions.

@patrick-kidger
Copy link
Collaborator

Installing from GitHub is the main option we support at the moment (we do need to put it on PyPI/conda already), so probably no imminent release.

@dhirschfeld
Copy link
Contributor Author

conda-forge usually prefer to create packages from pypi sdists however it's also possible to create packages from GitHub releases.

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

Successfully merging this pull request may close these issues.

Bump dependencies
4 participants