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

Auto update deps by default, except GPU pytorch & torchvision #2524

Merged
merged 3 commits into from Jan 28, 2024

Conversation

joeyballentine
Copy link
Member

For some reason I thought auto-updating was package level, but it actually is dep level. So, this change was easy and I could have done it a long time ago

@RunDevelopment
Copy link
Member

Why only pytorch deps?

@joeyballentine joeyballentine changed the title Auto update pytorch deps except pytorch itself Auto update deps except pytorch & torchvision Jan 27, 2024
@RunDevelopment
Copy link
Member

Uhmmm, this was a question... I don't see any consistency between the deps you do and do not auto update, so I would like to hear your reasoning.

@joeyballentine
Copy link
Member Author

As much as I'd like to just auto-update everything, this step happens before the UI is actually opened, making it a bad user experience if it takes too long. Even though we only update torch every once in a while, every once in a while having a bad UX experience of sitting on the splash screen for an hour while pytorch maybe updates (what if it just fails every time and locks you out? people tend to have issues with pytorch installs specifically) is not something i'd like to introduce.

NCNN already auto updates, so there was nothing to do there. ONNX i'm fine with also auto updating which is why i made that change. The other deps (like opencv) already also auto update.

Also, even when pytorch updates they usually dont introduce anything that would break or cause api changes. So not having it completely up to date if someone just doesnt update it isn't a dealbreaker. However, not having Spandrel up to date is, because we have been pretty consistently changing the API on that and therefore also changing how chaiNNer uses it, thereby causing errors to occur if it isn't updated. This was reported twice i believe after the most recent update came out that updated spandrel.

Of course, this will no longer be a concern under the extension system i want to eventually have. With that, code updates that change the code of the package's nodes would also be linked to the dependencies used, and therefore you'd never have code that wasn't compatible with the deps the code needs (i say never here but obviously there will be exceptions especially when people are using system python)

And, separately from that: If we ever get rid of the splash screen and allow chaiNNer to open up as dependencies/extensions download and install, then that also would alleviate the issue. We just don't have that right now either.

Therefore, this is the best solution imo.

@RunDevelopment
Copy link
Member

As much as I'd like to just auto-update everything

Then why auto updating opt-in instead of opt-out?

The other deps (like opencv) already also auto update.

Ah, I didn't know that everything chainner_standard auto updates.

@joeyballentine joeyballentine changed the title Auto update deps except pytorch & torchvision Auto update deps by default, except pytorch & torchvision Jan 28, 2024
@joeyballentine joeyballentine changed the title Auto update deps by default, except pytorch & torchvision Auto update deps by default, except GPU pytorch & torchvision Jan 28, 2024
@joeyballentine joeyballentine merged commit 7cce5ed into main Jan 28, 2024
14 checks passed
@joeyballentine joeyballentine deleted the auto-update-deps branch January 28, 2024 22:28
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.

None yet

2 participants