Skip to content

feat: add max_memory parameter to limit memory usage#83

Merged
p-e-w merged 4 commits intop-e-w:masterfrom
noctrex:master
Dec 11, 2025
Merged

feat: add max_memory parameter to limit memory usage#83
p-e-w merged 4 commits intop-e-w:masterfrom
noctrex:master

Conversation

@noctrex
Copy link
Contributor

@noctrex noctrex commented Dec 11, 2025

with this pull request we add the max_memory parameter, so that we can limit the memory we use on the specified devices.
for example if we use it in the config.toml as this:
max_memory = {0 = "16GB", "cpu" = "64GB"}
we will use at most 16GB of the first graphics card VRAM.

@noctrex noctrex changed the title add max_memory parameter to limit memory usage feat: add max_memory parameter to limit memory usage Dec 11, 2025
@noctrex
Copy link
Contributor Author

noctrex commented Dec 11, 2025

Closes #80

Copy link
Owner

@p-e-w p-e-w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

try:
max_memory = (
{
int(k) if k.isdigit() else k: v
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this happen automatically in Accelerate? And if not, why would we do it? The interface we offer should match what Accelerate does, not add conveniences on top.

Copy link
Contributor Author

@noctrex noctrex Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem that way, when I try without it, I get Trying dtype bfloat16... Failed (name 'max_memory' is not defined)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, then we shouldn't support string keys at all, if Accelerate doesn't.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah wait, is TOML the problem, because it doesn't support integer keys?

Copy link
Contributor Author

@noctrex noctrex Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It guess so, when I try to pass it as-is I get the error
Trying dtype bfloat16... Failed (Device 0 is not recognized, available devices are integers(for GPU/XPU), 'mps', 'cpu' and 'disk')

settings.model,
dtype=dtype,
device_map=settings.device_map,
max_memory=max_memory,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also needs to happen in reload_model below, otherwise abliteration will fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to add that, silly me

@noctrex noctrex requested a review from p-e-w December 11, 2025 14:29
}
if settings.max_memory
else None
)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to duplicate this code. You should be able to reuse the max_memory value stored in the model object. See the equivalent code for dtype a few lines up.

Copy link
Contributor Author

@noctrex noctrex Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really sorry for the mess, I'm a scatter brain today

@p-e-w p-e-w merged commit 740aab6 into p-e-w:master Dec 11, 2025
4 checks passed
@p-e-w
Copy link
Owner

p-e-w commented Dec 11, 2025

Merged, thank you!

@noctrex
Copy link
Contributor Author

noctrex commented Dec 11, 2025

Thank you, for this fantastic project!

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.

2 participants