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

[FEATURE] Update PyTorch dependency #884

Closed
sarthakpati opened this issue Jun 19, 2024 · 9 comments · Fixed by #919
Closed

[FEATURE] Update PyTorch dependency #884

sarthakpati opened this issue Jun 19, 2024 · 9 comments · Fixed by #919
Assignees
Labels
dependencies Pull requests that update a dependency file

Comments

@sarthakpati
Copy link
Collaborator

Is your feature request related to a problem? Please describe.

Now that PyTorch 2.3.0 has been out for a while, does it make sense to make the switch? There are a few backward incompatible changes [ref] which potentially relate to the work being done by @Geeks-Sid, so I will definitely wait for his comments.

Describe the solution you'd like

N.A.

Describe alternatives you've considered

N.A.

Additional context

Comments/suggestions, @VukW, @szmazurek?

@sarthakpati sarthakpati added the dependencies Pull requests that update a dependency file label Jun 19, 2024
@VukW
Copy link
Contributor

VukW commented Jun 28, 2024

Sorry as I am not proficient enough neither in the latest pytorch changes, nor in how GaNDLF uses distributed training. Do we have any tests for multi-gpu training? Cannot find any. If yes, then maybe just running a tests should be enough to ensure new version is ok for us.
Anyway, once in the future we would have to update dependency, so why not now

@sarthakpati
Copy link
Collaborator Author

Unfortunately, we do not have any GPU tests right now. 😞

I am fine with updating the dependency right now, but I would like to get the opinion of other developers/contributors/maintainers. 😄

@szmazurek
Copy link
Collaborator

Hey,
From my perspective why not, probably would be a matter re-running tests and making some corrections, as @VukW says would need to happen anyways.

@sarthakpati
Copy link
Collaborator Author

Sounds good, thanks!

Just waiting for @Geeks-Sid to respond and then we can start.

@Geeks-Sid
Copy link
Collaborator

Looks like the backwards compatibility issue does not affect us, however the tests might be good to be run on GPU's . We are good to go, but is there any issue in staying at current version?

@sarthakpati
Copy link
Collaborator Author

the tests might be good to be run on GPU's

Agreed - I am in discussion with a couple of CI providers to give us some extremely limited free GPU compute. Let's see how it goes.

is there any issue in staying at current version?

Nothing specific. Just that moving to the last stable release ensures that we aren't too far back in terms of ensuring latest bug fixes from PyTorch getting propagated forward. And since we will be making a jump with the new API branch anyway, I figured it might make sense to go to the latest one.

@szmazurek
Copy link
Collaborator

Dears, ragading the torch version - from version 2.2, torch has a built-in flash attention mechanism implemented, see: https://pytorch.org/blog/pytorch2-2/ . @sarthakpati mentioned that in the future we may integrate flash attention to speed up some models that employ the attention, this would be also useful regarding the synthesis module, where some diffusion models use it too. So, considering version updates, we may look directly into 2.2 as that solves both version update and flash attention.

@sarthakpati
Copy link
Collaborator Author

Since #845 also involves a torch version update, I think it might be best to let it get merged and tagged before working on this update.

@sarthakpati
Copy link
Collaborator Author

So, if there is no further issue with this, I am going to assign this is to @scap3yvt to start work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants