fix: make reset_model null-safe to handle study cancellations (#77) by umran666 · Pull Request #367 · p-e-w/heretic

umran666 · 2026-06-10T03:51:40Z

When a study is cancelled (via Ctrl+C) during a model reload, self.model is left as None. Subsequent calls to reset_model() fail with an AttributeError when trying to read self.model.config or self.model.dtype.

This PR introduces a cached self.dtype attribute on the Model class, populates it upon successful load, and updates reset_model() to safely check for nullity and fall back to the cached self.dtype when self.model is None. This allows the CLI to recover and successfully reload the model when selecting a new trial after a cancellation.

gemini-code-assist

Code Review

This pull request updates src/heretic/model.py to safely track and handle the model's dtype, especially when self.model is None during model resets. The review feedback suggests two improvements: first, to safely access the model's configuration using nested getattr to prevent potential AttributeErrors, and second, to fall back to the user's configured dtypes instead of a hardcoded "auto" string when the model's dtype is unavailable.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

gemini-code-assist · 2026-06-10T03:52:40Z

+        current_model = (
+            getattr(self.model.config, "name_or_path", None)
+            if self.model is not None
+            else None
+        )


To prevent a potential AttributeError if self.model does not have a config attribute (for example, in certain mock or custom model configurations), it is safer to use getattr to access config as well.

Suggested change

current_model = (

getattr(self.model.config, "name_or_path", None)

if self.model is not None

else None

)

current_model = (

getattr(getattr(self.model, "config", None), "name_or_path", None)

if self.model is not None

else None

)

gemini-code-assist · 2026-06-10T03:52:40Z

+        if self.model is not None:
+            dtype = self.model.dtype
+            self.dtype = dtype
+        else:
+            dtype = self.dtype or "auto"


If the initial model load was cancelled, self.dtype will be None. Falling back to a hardcoded "auto" string might bypass the user's custom dtypes configuration (defined in self.settings.dtypes). It is safer to fall back to the first configured dtype in self.settings.dtypes if available, and only use "auto" as a final fallback.

Suggested change

if self.model is not None:

dtype = self.model.dtype

self.dtype = dtype

else:

dtype = self.dtype or "auto"

if self.model is not None:

dtype = self.model.dtype

self.dtype = dtype

else:

dtype = self.dtype or (self.settings.dtypes[0] if self.settings.dtypes else "auto")

p-e-w

Thanks for tackling this problem, though I think this PR can be substantially simplified.

BTW, the PR description is mangled with missing spaces, presumably from copy&paste or a broken agent. I don't mind contributors using LLMs (although I don't use them myself), but please do at least a basic sanity check before clicking "submit".

p-e-w · 2026-06-10T11:22:41Z

+        else:
+            dtype = self.dtype or (
+                self.settings.dtypes[0] if self.settings.dtypes else "auto"
+            )


Why not simply always user self.dtype here? It's always set, and it's always the right dtype, no?

Good catch. I just pushed an update to use self.dtype directly.

p-e-w · 2026-06-10T11:22:55Z

                    **self.revision_kwargs,
                    **extra_kwargs,
                )
+                if self.model is not None:


How can self.model be None here?

Oh yea It can't be. from_pretrained never returns None. I've removed the redundant None checks in the latest commit.

…dtypes

umran666 · 2026-06-10T15:48:35Z

Ah, my apologies for the formatting. You're completely right—a local tokenizer bug stripped the spacing around the inline code backticks when the description text was formatted.

I've edited the PR description to restore the spaces, and pushed the updates to simplify the code changes as we discussed. Thanks for catching that.

p-e-w · 2026-06-10T15:49:28Z

    def __init__(self, settings: Settings):
        self.settings = settings
        self.needs_reload = False
+        self.dtype = None


I don't think we need this either. The dtype will always be set below, otherwise an exception bubbles up.

Makes sense. Removed the redundant initialization.

p-e-w · 2026-06-10T15:50:04Z

            **self.revision_kwargs,
            **extra_kwargs,
        )
+        self.dtype = self.model.dtype


We don't need this. The dtype must remain constant throughout the run.

Agreed. Removed this re-assignment since self.dtype is already set from the initial load.

p-e-w · 2026-06-10T15:50:33Z

-        dtype = self.model.dtype
+        # self.dtype is populated on successful load in __init__ or reload,
+        # providing a safe fallback if the model object is currently None.
+        dtype = self.dtype


No need for a separate variable, just inline self.dtype below.

Done, inlined self.dtype directly.

p-e-w · 2026-06-10T15:54:10Z

        if current_model == self.settings.model and not self.needs_reload:
-            # Reset LoRA adapters to zero (identity transformation)
+            # Reset LoRA adapters to zero (identity transformation).
+            assert self.model is not None


Not sure what the purpose of this assertion is. This condition is guaranteed to be true by the logic above, because if self.model is None, current_model is also None, and this if branch will never be taken because self.settings.model cannot be None.

You're right, that was completely redundant. I've removed the assertion.

p-e-w · 2026-06-11T05:12:31Z

    # Set for multimodal models, None for text-only ones.
    processor: ProcessorMixin | None
    peft_config: LoraConfig
+    dtype: torch.dtype | str | None


No, it can only be a torch.dtype. That's the type of model.dtype according to the docs, and it can never be None.

Understood. I've updated the Model.dtype type annotation to strictly be torch.dtype and confirmed it passes ty check.

p-e-w · 2026-06-11T05:25:04Z

    "tomli-w~=1.2",
    "tqdm~=4.67",
    "transformers[kernels]~=5.6",
+    "torch",


Why did you add this dependency?

Please don't let agents blindly do stuff. It's such a hassle to review.

Apologies for the noise. The package and lockfile modifications were accidental environment pinning changes introduced by the coding agent during local test setup. I have reverted pyproject.toml and uv.lock to match upstream master exactly, leaving the PR focused solely on the cancel-reload bug fix.

p-e-w · 2026-06-11T05:36:06Z

Merged!

fix: make reset_model null-safe to handle study cancellations (p-e-w#77)

85ca6c3

gemini-code-assist Bot reviewed Jun 10, 2026

View reviewed changes

p-e-w reviewed Jun 10, 2026

View reviewed changes

fix: address bot review, use nested getattr and fallback to settings …

b0dac6f

…dtypes

umran666 force-pushed the fix/study-cancellation-reload branch from 32d462e to b0dac6f Compare June 10, 2026 15:29

fix: address maintainer review comments in model.py

496d00e

p-e-w reviewed Jun 10, 2026

View reviewed changes

fix: address maintainer review feedback on reset_model

8dda10b

p-e-w requested changes Jun 11, 2026

View reviewed changes

fix: update Model.dtype type annotation to torch.dtype

b1bab4d

p-e-w reviewed Jun 11, 2026

View reviewed changes

chore: revert pyproject.toml and uv.lock changes

6127af0

p-e-w merged commit e735203 into p-e-w:master Jun 11, 2026
4 checks passed

Conversation

umran666 commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

p-e-w left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

umran666 commented Jun 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

p-e-w commented Jun 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

umran666 commented Jun 10, 2026 •

edited

Loading