Skip to content

[Community Discussion] Scheduler design #311

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

Closed
patrickvonplaten opened this issue Sep 1, 2022 · 9 comments
Closed

[Community Discussion] Scheduler design #311

patrickvonplaten opened this issue Sep 1, 2022 · 9 comments
Labels
stale Issues that haven't received updates

Comments

@patrickvonplaten
Copy link
Contributor

This is a very nice PR by @pcuenca showing what changes need to be done to the PNDM/PMLS scheduler to make it work with JAX/XLA - it's actually more then anticipated and shows that the scheduler now substantially differs from the original implementation that we use for PyTorch.

In the coming days we will integrate these changes into main diffusers to make the library compatible with Flax/JAX. Now the big question is should we:
a) Make each scheduler very generic and continue the set_format("pt") logic? While this would make sense logically as the schedulers don't store any trainable weights really - this could potentially lead to quite some if - else statements and too much abstracted code, e.g. lots of self.where(...) functions in scheduler_utils.py. Also maybe we want schedulers to have trainable weights in the future? Also do we anticipate schedulers to be more or less complex in the future?
b) Make one scheduler file for each framework. Instead of trying to fit all frameworks into one scheduler file, we make one scheduler for one framework. The advantage is clearly readability. Also most people probably always only work in one framework so for them it might be nicer to have schedulers seperate. However: Some schedulers will probably be 1-to-1 the same (which also might not be a problem necessarily)

I'm starting to be lean more and more towards b) actually here.

Would love to discuss - cc @anton-l @patil-suraj @natolambert @pcuenca

@patil-suraj
Copy link
Contributor

I agree with your points, creating a generic scheduler here, will complicate a the overall design. In JAX we need to take special care of state, keeping everything scheduler as jnp arrays, avoid device to host communication etc. So I'm also in favor of option b. B is good for both readability and maintainability.

@natolambert
Copy link
Contributor

Having done a bunch of work (maybe the most out of us) on trying to numpy-ify schedulers and the related tests, I think option b) will also result in much more accessible code. If we lay out the directory correct, it'll be very obvious what the different files do, and fitting in with the style designs of the library: one-file one purpose.

When trying to make them work with numpy it generally is a lot of self.function and if-else statements. Then you need to add tests for each option anyways because the random number generation is handled differently in each.

I'll go look at the PR now.

@vishnu-anirudh
Copy link
Contributor

Hello
I hope it is okay for me to pitch in my two cents.
I prefer option b as well. It is very difficult to achieve a generic scheduler now. Given that this is an evolving field, it is easier to dedicated for every framework.
However, from interface perspective, it would be better if each of the scheduler implements an Abstract Base Class. This gives a good template on how the scheduler can be accessed/used when it is imported in other code. Also, as the diffuser library evolves, it becomes easier to develop functions/classes that can take in multiple schedulers (the type-hint would be the abstract base class).
In this way, we won't have complicated one-scheduler-fits-all situation but at the same time we have a basic interface which will be consistent for all the implemented schedulers, this will improve the maintainability and the readability of the code base.

@patrickvonplaten
Copy link
Contributor Author

Cool let's go for separate schedulers then!

@vishnu-anirudh
Copy link
Contributor

Just out of curiosity, can I ask how you design the software architectures for the libraries? Do you do that internally or are there sprints where the community gets involved?

@patrickvonplaten
Copy link
Contributor Author

What exactly do you mean by software architectures? Like the whole library here? Usually we start a library internally but try to include the community in design decisions as quickly as possible :-)

@vishnu-anirudh
Copy link
Contributor

Yes meant the whole library. Okay makes sense. Thank you.

@patrickvonplaten
Copy link
Contributor Author

Cool design taken -> we'll have framework-dependent schedulers :-)

@github-actions
Copy link
Contributor

github-actions bot commented Oct 8, 2022

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 8, 2022
@github-actions github-actions bot closed this as completed Nov 2, 2022
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

No branches or pull requests

4 participants