Skip to content

Fix raise NotImplemented to raise NotImplementedError in wrap_llm_lora - #1259

Closed
Mr-Neutr0n wants to merge 1 commit into
OpenGVLab:mainfrom
Mr-Neutr0n:fix/raise-not-implemented-error
Closed

Fix raise NotImplemented to raise NotImplementedError in wrap_llm_lora#1259
Mr-Neutr0n wants to merge 1 commit into
OpenGVLab:mainfrom
Mr-Neutr0n:fix/raise-not-implemented-error

Conversation

@Mr-Neutr0n

Copy link
Copy Markdown

Summary

  • NotImplemented is a built-in constant used for rich comparison methods, not an exception class. Raising it directly produces a confusing TypeError: exceptions must derive from BaseException instead of the expected NotImplementedError.
  • This fixes the else branch in wrap_llm_lora() to properly raise NotImplementedError with a descriptive message indicating which LLM architecture is unsupported.

Before (broken):

raise NotImplemented
# TypeError: exceptions must derive from BaseException

After (fixed):

raise NotImplementedError(f'Unsupported architecture: {self.llm_arch_name}')
# NotImplementedError: Unsupported architecture: SomeModel

Test plan

  • Verified that raise NotImplemented produces TypeError in Python
  • Confirmed this is the only instance of this pattern in the codebase
  • The fix follows the same pattern used elsewhere in the file (e.g., line 84)

`NotImplemented` is a built-in constant intended for use in rich
comparison methods, not an exception. Raising it directly produces a
confusing `TypeError: exceptions must derive from BaseException` instead
of the expected `NotImplementedError`.

This change fixes the error in `wrap_llm_lora()` to properly raise
`NotImplementedError` with a descriptive message showing which
architecture was unsupported.
@Mr-Neutr0n

Copy link
Copy Markdown
Author

Closing this one to tidy up my open pull requests.

It's been open around five months with no review activity, and I'd rather withdraw it than leave it sitting in your queue. Nothing needed from you, and no hard feelings at all.

If the fix is still wanted, this can be reopened, or I'm happy to redo it properly against current main. Apologies for the noise.

@Mr-Neutr0n Mr-Neutr0n closed this Jul 28, 2026
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.

1 participant