-
Notifications
You must be signed in to change notification settings - Fork 3
reasoning output #9
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the code looks fine some comments, I will review again after you resolve. I think next time we should also try to break PRs up, 500 something lines is a bit harder to review (but okay this time :))
src/nuggetizer/core/llm.py
Outdated
| # print(f"🔍 DEBUG LLM: API call completed successfully") # not | ||
| # removed because it's very helpful for debugging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I think we should remove these even if it is helpful for debugging, coding practice wise it is for the developer to debug.
- We could have a --debug mode where we expose such things, but that is a separate PR. And we shouldn't be doing print within function methods, logging is a better alternative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see let me remove this then. We can do debug mode in another PR if needed
src/nuggetizer/core/llm.py
Outdated
| # print(f"🔍 DEBUG LLM: Full response: {completion}") # not | ||
| # removed because it's very helpful for debugging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
src/nuggetizer/core/llm.py
Outdated
| response = reasoning_content if reasoning_content else "" | ||
| reasoning_content = message["reasoning_content"] | ||
| else: | ||
| print(f"No reasoning found in response from {self.model}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be printed? Feels spammy. if the model is a reasoning model and we hit here, we reach some error state and should warn. else this should not be printing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is also debugging print. removed
src/nuggetizer/core/llm.py
Outdated
| "qwen" in self.model.lower() | ||
| or "qwen2" in self.model.lower() | ||
| or "qwen3" in self.model.lower() | ||
| ): | ||
| # Use cl100k_base for Qwen models as they typically use | ||
| # similar tokenization | ||
| encoding = tiktoken.get_encoding("cl100k_base") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's not conflate this into this PR, if we don't know the model just don't include the encoding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good. removed
| if self.log_level >= 1: | ||
| self.logger.info( | ||
| f"Initialized Nuggetizer with models: {creator_model}, {scorer_model}, {assigner_model}" | ||
| ) | ||
| if log_level >= 1: | ||
| self.logger.setLevel(logging.INFO) | ||
| if log_level >= 2: | ||
| self.logger.setLevel(logging.DEBUG) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's this code doing, we don't update log_level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can specify log level in the input. same debugging logs appear only if we specify log level to be 2
I see thanks for pointing out. I will break them into smaller PRs |
ronakice
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you :)
This PR enables reasoning output.