11 ms·
Revert for jart’s llama.cpp MMAP miracles
- ukuina 4y agoCan someone in the know describe what the hullaballoo is about? Seems like ego-driven optimization that breaks compatibility?
- femboy 4y agoWhat's ego-driven optimization?
- oh_sigh 4y agoPushing optimizations for personal glory that look good with some benchmarks but may have unthought of or hidden regressions on other aspects of the code/user base. On the same hand this may be an ego driven revert. from reading the bug it seems like some people might be salty that jart gets a lot of publicity for a few changes where other people which bigger contributions to the project don't get.
- simion314 4y agoThe issue was with the model versioning compatibility change and someone inserting his initials in the magic number. The optimizations worked great for me on Linux, the first tiem your run the CLI program you have to wait a lot of time, more then 1 minute for a 20Gb model , but the next runs are instant. Maybe some Windows guys are salty that this does not work as great on their OS , if that is the case they should update it to get the performance boost. This are not soem nanoseconds you gain but actual minutes for each run.
- trsohmers 4y agoThe tl;dr as I understand it is that jart had a misunderstanding of how what was actually happening and the benefits of the map optimization… the claims of actually being able to shrink the model size from 20GB > 6GB were just completely false, and while there was a model loading time improvement, actual memory required and used did not change. A number of people saw this and said that making a breaking change to the repo that a lot of people are using and have forked for other models was a bad idea, thus this new PR.
- blibble 4y agohere mmap is being used for essentially lazy loading that's it
- chpatrick 4y agoBut then it's pretty much the same as using the old version and turning on swap, so I don't really see the point. As far as I understand the whole model needs to be read constantly so there's no benefit from the random access mmap provides.
- sitkack 4y agoSwap is a system level property not a program level property. They are similar, use similar mechanisms, but the experience that a user would see are very different.
- chpatrick 4y agoI'm not sure it would be that different but mmap has the benefit that it can swap directly to the model file on the disk instead of making a copy in swap space.
- sitkack 4y agommap is like tightly scoped, targeted swap. If the PoR is on disk, the OS is free to reclaim that memory for something else at anytime. It really is a beautiful hack, but if you turn on swap for the system in that way, it has to balance memory usage across all the running programs. In this use of mmap, there is nothing to swap out as the source of truth is always on disk. During general swap usage, memory has to travel in both directions.
- simonw 4y agoThe 20GB to 6GB confusion appears to have come from the title of the Hacker News post the other day: https://news.ycombinator.com/item?id=35393284 https://news.ycombinator.com/item?id=35393284 The PR it linked to said nothing of the sort: https://github.com/ggerganov/llama.cpp/pull/613 https://github.com/ggerganov/llama.cpp/pull/613
- augusto-moura 4y agoIs not ego-driven, the requirements for running any model dropped by more than half, you can know run the largest model, 30b, on domestic over the shelf computers. The change is very welcome in the community
- tempaccount420 4y agoThere were no improvements to memory use, the earlier GBs were a measurement error. If you couldn't run a model before (or were swapping, so running very slow), then you still have the same problem. You will actually "swap" a lot more than before if you have barely enough memory, but this is fixable with the --mlock flag. Edit: For everyone downvoting, please tell me what is wrong with my comment. I don't have a bone in this fight.
- xiphias2 4y ago4chan came here to chime in a bit. You were downvoted by 10 year old children, good luck explaining to them how memory locking works :)
- klohto 4y agommap breaks previous (not guaranteed) compatibility and (few) people demand option to turn it off by reverting all of the commits and throwing hands up by demanding more testing, documenting, and more optional arguments. While I agree with some of the premises, it’s up to ggerganov in the end. If he wants this to be the default, so be it. Throwing a (polite) tantrum because you should please all, while not offering to do any of the work, is entitled demand.
- 4bpp 4y agoA project that has been generating a lot of buzz lately (CPU-based inference for Facebook's LLaMa model that works on commodity hardware) has attracted contributions from a tech/activist celebrity (https://en.wikipedia.org/wiki/Justine_Tunney https://en.wikipedia.org/wiki/Justine_Tunney). Their somewhat overly self-assured/-aggrandizing style (e.g. Github posts written in a tone like they run the place, changing the file format magic number to include their own initials along those of the project's originator) rubbed many people the wrong way, and a sweeping change they introduced may have resulted in performance regressions for several users (while also being hugely oversold: a fantastical and quickly disproven claim about significantly reduced memory usage sat at >1k upvotes on HN yesterday). Then another long-standing contributor made a PR just seeking to flat out revert the patch in question. The discussion quickly turned toxic, with a (thankfully) not-yet-quite-vocalised US culture war undercurrent and people bandwagoning based on their personal disposition towards the person at the core of it.
- fastball 4y agoAlso worth mentioning that there is some level of controversy over how much of this work involving mmap should be attributed to jart vs slaren. Slaren originally authored a PR using mmap which some people are claiming was the much better implementation of the feature (including not needing to change the model format) and jart basically re-wrote it so that she could take credit for it.
- detaro 4y ago> basically re-wrote it so that she could take credit for it. compare the actual PR: https://github.com/ggerganov/llama.cpp/pull/613 https://github.com/ggerganov/llama.cpp/pull/613 You'll note that it includes the original commits from Slaren, explicitly mentions the collaboration with Slaren and explicitly requests to preserve these commits on merge.
- augusto-moura 4y agoWhy reverting it instead of adding upon it? The author of the revert could easily start working on reintroducing the previous format behind a flag. AFAIK llama.cpp is not even v1 yet. I see this revert PR as unnecessary
- jimbob45 4y agoFeature flags are great but they should not be used as a crutch to leave unfinished or non-working code in the codebase. IMO they should be used to rapidly pull back the change before the slower revert can take place.
- jasonjmcghee 4y agoI’m not in the know at all here, but the original PR wasn’t purely additive- there was code deletion, and additions across a number of files. It seems to change the checkpoint format. The code should be abstracted differently for it to be placed behind a flag.
- augusto-moura 4y agoI understood that, but it was accepted. We don't need to cry over the spilled milk, one can re-add the previous model based on the removal PR. No need to push a revert
- shakow 4y agoI really dislike giving HN exposure to this kind of issue; it only brings us the forbidden pleasure of voyeurism while not helping the maintainers & contributors in the slightest – and can even crystallize conflicts while we eat popcorn. Let us let them take their time, wash their dirty laundry among themselves, and take the time they need to go forward on the project.
- ftxbro 4y ago[flagged]
- shakow 4y agoPlease tell me what people who would discover this thread on HN could constructively add to this conversation. > Real discussion and transparency can involve multiple viewpoints that conflict with each other while each having their plusses and minuses. Which is exactly what's currently happening between the concerned people in the GH issue.
- ftxbro 4y ago> Please tell me what people who would discover this thread on HN could constructively add to this conversation. I mean is that really the bar for posting hacker news articles? > > Real discussion and transparency can involve multiple viewpoints that conflict with each other while each having their plusses and minuses. > Which is exactly what's currently happening between the concerned people in the GH issue. Yeah that was my point.
- michaelmrose 4y agoAt least one reasonable bar is not effectively vandalizing other communities by airlifting in a bunch of uninvolved commenters into an already drama filled situation.
- aww_dang 4y agoIdeally, this will be resolved diplomatically by someone who knows how to handle these personalities. From that point we might learn from their example.
- AceJohnny2 4y ago> > > memory mapping means that the model will stay behind and eat your memory even after the process is closed > > I don't think I'm unterstanding this right: You're saying that memory will not be freed by the OS after the process terminates? > You're understanding it perfectly. The whole raison d'etre for mmap() is the ability to leave stuff in RAM (or swap, albeit if that happens it's completely detrimental to this use case) unlinked from the process itself. Basically it's just storing a file/memory block in RAM which can be accessed from multiple processes. Wow, does this developer not understand the nuances of mmap().
- deleted 4y ago[deleted]
- vore 4y agoIf you squint maybe you can argue that mmap will leave things in the page cache. But, you know, it doesn’t matter and not even munmap will save you there so I have no idea what they’re getting at.
- jcranmer 4y agoThe fundamental operation of mmap is to add new entries to the page table of a process, and the precise properties of those entries are heavily dependent on what the arguments to mmap are. When you mmap a regular file, you're essentially adding an entry to the page table that shares the data with the kernel's filesystem cache. I think he was trying to explain the implications of this fact, but doing so in an incredibly garbled manner, and getting his conclusions wrong. There are performance implications to using mmap (not always good, not always bad), but both sides of the discussion here immediately dug their heels in on their conclusion without anyone trying to do any analysis to see what the actual implications were, and why.
- AceJohnny2 4y ago> I think he was trying to explain the implications of this fact, but doing so in an incredibly garbled manner, and getting his conclusions wrong. Yeah. I like using the expression "knows just enough to be dangerous" (usually applied in humility to myself), and this situation is such a perfect example. Someone who seems to know just enough about the advanced workings under mmap() to completely misunderstand the implications.
- Jasper_ 4y agoThis contributor doesn't appear to know how mmap works if they're claiming the only benefit is sharing data between processes (what? MAP_PRIVATE mappings aren't shared), and that memory is leaked after the process exits. There are a lot of thorny issues with mmap, and I'm sure there are legitimate regressions with the approach and things to be fixed, but it sure would be nice to see an analysis from someone who actually knows what mmap is.
- deleted 4y ago[deleted]
- patrakov 4y agoThe code is related to MAP_PRIVATE mappings of the same file that are not written to. Such mappings are effectively deduplicated and thus occupy the RAM once no matter how many processes map the file.
- tempaccount420 4y agoEditorialized title. Please rename it to "Bring back the ggml model format and revert breaking mmap change (#613)" @dang
- d23 4y agoThe only editorialized bit is "miracles," in my opinion. The current title gets closer to including the relevant context than does your updated title.
- NortySpock 4y agoThis is someone angrily filing a revert-all PR due to a performance regression, rather than helping diagnose the issue or make it configurable. Don't bother reading. It sounds like one person experienced a performance regression as a result of the llama.cpp MMAP changes, and decided to create a pull request to revert all of those changes. While they propose wrapping the mmap changes behind a feature flag / command-line flag, that's not what this PR does -- it just reverts the original commits. It's a "tear it down NOW" reaction rather than a "how can I improve this" reaction. jart and a few other people in the PR have now proposed a variety of feature-flags or forked versions to address the issue in a more nuanced way.
- eska 4y agoThat’s not what’s happening. File format compatibility is broken while performance degrades by 10x for some people.
- cuuupid 4y agoI don’t understand the controversy in this issue. It seems they could have saved a lot of time spent throwing shade back and forth by just implementing a feature flag. The argument against the feature flag is ultimately more egregious; it’s an experimental feature, breaks compatibility, decreases memory usage for a fair portion of the population while 10x’ing load speed for the rest so very YMMV for an optimization. In another project this wouldn‘t even be a revert PR but just a PR to feature flag it. Can’t help but notice that more than half the replies on this PR are from people who have a limited understanding of LLMs, admit to it, and are just adding noise because this project is popular right now. First step would be to lock this contributors only so they can get a more streamlined discussion going.
- Aeolun 4y agoIn my experience, there will always be a population of developers/users that prefer for things to always stay the same (and therefore never break). Unfortunately that means never improving.
- seydor 4y agoSomeone should use GPT3 and a voice model to make a dramatic version of github issues.
- surteen 4y agoggerganov commented Apr 2, 2023 So this is pretty stupid - I just lost my Sunday trying to figure out how to salvage this stupid drama @anzz1 and @jart You are no longer welcome as collaborators to the project.
- fastball 4y agoWhat is the purpose of re-posting a comment in the linked thread here verbatim?
- Operyl 4y agoUpdated context for those who read the issue before it was posted, that wasn’t there during the time of the original submission.
- qwertox 4y agoBut you should quote it completely, else it's manipulation: > You are no longer welcome as collaborators to the project. I know you really care about it and only doing it because you really want to make it better - I'm 100% sure about this. But in fact, you are doing the opposite. And if you fail to see this - I'm sorry
- xiphias2 4y agoIt's really bad reply from Greg. He's the owner of the project, he has the power to accept / not accept changes, and he didn't object to the version change, now he pushes responsibility to the contributors. It's ugly way of dealing with other people. The way to solve this situation is to set up a video call between them to deescalate the emotional part of the situation (which is not a big deal anyways, we can wait a few days for the technical details to get resolved).
- jart 4y agoThen he'd have to do a video call between him, myself, and 4chan.
- sitkack 4y agoSo much snark! Here and in the issue. Programming is hard enough.
- superkuh 4y agoThe gist is that people runing proprietary operating systems without the capability don't benefit from the change and don't want it to be the default.
- thedonkeycometh 4y ago[dead]
- michaelmrose 4y agoSo this matches a pattern of submissions that eventually is or should be flagged. Can we not post GitHub issues that represent inter-project drama? It's not the discussion here that is the problem its effectively bringing commenters FROM here to stir up drama there where tensions are already high.
- thedonkeycometh 4y ago[dead]
- _gabe_ 4y agoThis is the part of Open Source I really despise. It looks like the top contributors in this repository have contributed a few hundred lines of code (as opposed to the 20Kloc by the author). I understand that lines of code is not comparable to level of effort, but there is at least some level of correlation there. The predominant attitude I have seen with my open source projects is entitlement and anger at decisions I have made, (whether that's because my license isn't MIT or because I don't want to use the latest and greatest features of language X, or because I use 2 spaces instead of 4). I just want to share my code, but some people make this unnecessarily difficult and want to cause drama where there never needed to be any. Now, with that said, I have also met amazing people who have offered invaluable insights. These people have made contributions, to code and discussions, and on the other side provided amazing libraries and support. I really love Open Source, but there is a certain aspect of the community that can be downright hostile, and I hate that. I never understood why some developers feel the need to belittle others or to scoff at what other people want to share. I hope that if people know my name it's because I encouraged them and gave them help and/or praise for a cool project, and not because I was a dick and made them feel like crap for an inconsequential action.