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

Replace torchsde dependency with torchsde-brownian #5192

Closed
wants to merge 1 commit into from

Conversation

akx
Copy link

@akx akx commented Sep 26, 2023

What does this PR do?

This PR replaces the torchsde dependency with my fork torchsde-brownian that only includes the Brownian motion code that is required by the dpmpp_sde sampler (that itself is lifted from k-diffusion (where there is a sibling PR crowsonkb/k-diffusion#82)) that was added in #3020.

The fork also has one dependency less (boltons), though that change would also be in a future unreleased version of torchsde thanks to my PR over there.

The implementation of this PR here is really just a search-and-replace.

Before submitting

  • Did you read the contributor guideline?
  • Did you read our philosophy doc (important for complex PRs)?
  • Did you make sure to update the documentation with your changes?
    • No documentation changes, just an internal library switcharoo.
  • Did you write any new necessary tests?
    • No new tests, the brownian code is exactly the same as in the original torchsde package.

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@patrickvonplaten
Copy link
Contributor

Hey @akx,

Thanks for the PR! Just to better understand is there any functionality that is changed? If the PR stays exactly the same, I don't fully see the advantage of changing the package here tbh

@akx
Copy link
Author

akx commented Sep 27, 2023

@patrickvonplaten The functionality (and the code, aside from import names of course) is exactly the same, but there's 70% less code (since the ODE parts of the torchsde package that aren't used aren't there anymore):

$ tokei torchsde/
 Language            Files        Lines         Code     Comments       Blanks
===============================================================================
 Python                 32         3725         2494          691          540
$ tokei torchsde_brownian/
 Language            Files        Lines         Code     Comments       Blanks
===============================================================================
 Python                  6         1130          755          210          165
$

You can see how this came to be in akx/torchsde-brownian#1.

There's also one less transitive dependency as torchsde_brownian doesn't depend on scipy, though of course other bits in diffusers might and do.

This also ties in to the philosophy doc:

Diffusers aim at being a light-weight package and therefore has very few required dependencies

torchsde itself also had packaging issues (that were resolved yesterday partly due to yours truly), which caused repeated pip warnings for anyone who had installed the package.

@akx
Copy link
Author

akx commented Sep 27, 2023

Anyway, there's also a secondary concern here since #3020 basically just copy-pasted the sampler from k-diffusion (which, confusingly, this repo also depends on anyway), and seemingly re-licensed that MIT code as Apache-2.0 here.

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

@github-actions github-actions bot added the stale Issues that haven't received updates label Oct 30, 2023
@akx
Copy link
Author

akx commented Oct 30, 2023

De-staling. WDYT, @patrickvonplaten?

@patrickvonplaten
Copy link
Contributor

@akx, I don't feel super comfortable replacing the dependency because torchsde is a robust, well-known library with >1K stars. I don't really mind that it has some additional functionality that we don't need

@akx
Copy link
Author

akx commented Nov 2, 2023

@patrickvonplaten Thanks for getting back to me!

As explained in the PR description and torchsde-brownian's readme, torchsde-brownian is a "cropped" version of torchsde proper, with no code changes. It's just as robust as torchsde itself. (torchsde's authors have noted that they no longer work on the package, for what it's worth: google-research/torchsde#131 (comment))

Either way, I suppose this PR is no longer quite as required now that torchsde==0.2.6 (likely the final ever version, given the comments above) is out after I fixed torchsde's CI process to let them release with confidence (google-research/torchsde#140).

The secondary concern still stands: the sampler introduced in #3020 (that introduced the torchsde requirement) was copy-pasted from the k-diffusion project (which has a different license than this project).

Copy link

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

@github-actions github-actions bot closed this Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Issues that haven't received updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants