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

refactor(Klipper): trim klipper git clone to only latest history #418

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nberardi
Copy link

@nberardi nberardi commented Jan 1, 2024

I was receiving the following error on my Raspberry Pi Zero 2 W during the installation of Klipper, while running the 64-bit lite version of Raspberry Pi OS (bookworm).2

error: RPC failed; curl 92 HTTP/2 stream 0 was not closed cleanly: PROTOCOL_ERROR (err 1)

I tracked this down to a likely low memory issue due to the size of the git repo. I was able to correct for this issue, by only downloading the tip of the history of the Klipper repo during install by setting --depth 1 in the git clone command. This resulted in a roughly 13x reduction in the size of the downloaded repo and allowed me to continue my install without issue using KIAUH.

@dw-0 dw-0 changed the title trim klipper git clone to only latest history refactor(Klipper): trim klipper git clone to only latest history Jan 1, 2024
@dw-0
Copy link
Owner

dw-0 commented Jan 1, 2024

I think this might be a good change. I tested it myself and there is roughly a saving of 140mb we don't need to download, just 10mb instead.

Though, there is another feature of KIAUH which needs to be refactored a little bit if we implement that change. There is a "rollback" feature, where its possible to enter a number and klipper gets "rolled back" (basically a hard reset) by that amount of commits. https://github.com/dw-0/kiauh/blob/master/scripts/rollback.sh#L71

If we have "no history" or a very short one, and for some reason someone want to rollback a larger amount of commits than available in the history, git will throw an error. so it may require refactoring that workflow, so that the size of the history is checked before the hard reset is executed.

@ElectricalBoy
Copy link

Doing a shallow clone is more expensive than doing a full clone in the long term (ref: CocoaPods/CocoaPods#4989 (comment), Homebrew/brew#9383).

Sure - Kilpper repo will (almost certainly) never get as much traffic as the two repos linked above (nor will its size grow as big), but my point still stands. Doing a full clone initially becomes cheaper in the long run (especially if you run updates).

@nberardi
Copy link
Author

@ElectricalBoy more expensive than not being able to pull the repo on my Raspberry Pi Zero 2W? 😀

There are other options that could have been tried, but this was the cleanest code change that allowed me to keep moving on my install.

@ElectricalBoy
Copy link

I see the change of the default behavior to introduce a long-term side-effect as a regression.

Yes, I do acknowledge that doing a shallow clone does act as a short-term solution for this specific problem, especially on lower-end devices (i.e., RPi Zeros). However, this comes at a cost of introducing a regression on the majority of use cases (i.e., normal RPis). This, I believe, shows that the cleanest code is not the cleanest solution for a problem.

What I think should happen instead is keeping the default behavior to do a full clone but adding an option to do a shallow clone (which can possibly be set in the advanced menu).

@dw-0
Copy link
Owner

dw-0 commented Jan 21, 2024

Doing a shallow clone is more expensive than doing a full clone in the long term

I understand it that way, that it would be more expensive on githubs side than on the client side?

I see the change of the default behavior to introduce a long-term side-effect as a regression.
[...]
However, this comes at a cost of introducing a regression on the majority of use cases

Can you elaborate? What would be an actual regression?

@dw-0
Copy link
Owner

dw-0 commented Feb 4, 2024

@ElectricalBoy any more input on this? Especially in regards to my previous questions?

@ElectricalBoy
Copy link

I understand it that way, that it would be more expensive on githubs side than on the client side?

If the user needs to checkout an old commit (that is not available via the shallow tree) then fetching the giant tree eventually becomes inevitable - and this connects to my old regression argument, as this introduces the new overhead when rolling back.

Although I am mildly annoyed by the regression, considering the # of times a rollback is needed in general this seems like a reasonable compromise after giving some more thought. Plus it seems that Git handles this better in general than how it did few years back.

P.s.: Sorry for getting back so late - had other commitments that needed my attention.

@Sineos
Copy link
Contributor

Sineos commented Jul 17, 2024

Since I ran into the issue as well: There are two ways to work around this issue:

  • Increase the SWAP size
  • Modify the memory GPU / CPU memory split. GPU memory is rarely needed on headless anyway

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.

4 participants