fix(oocana): prevent oomol_llm_env property side effect on repeated access#461
fix(oocana): prevent oomol_llm_env property side effect on repeated access#461leavesster wants to merge 1 commit intomainfrom
Conversation
Property was calling send_warning on every access, which violates property semantics. Now only warns once on first access using a cached flag.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes a side effect issue in the oomol_llm_env property by preventing repeated warning messages on multiple accesses. The property previously sent warnings every time it was accessed, which violates the principle that properties should be idempotent and side-effect free.
Changes:
- Added
__oomol_llm_env_warnedflag to track whether warnings have been sent - Modified the property to only send warnings on first access by checking the flag
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not self.__oomol_llm_env_warned: | ||
| self.__oomol_llm_env_warned = True | ||
| for key, value in oomol_llm_env.items(): | ||
| if value == "" or value == []: |
There was a problem hiding this comment.
The comparison value == [] on line 318 will never match the "models" value when the environment variable is empty or not set. When OOMOL_LLM_MODELS is empty, "".split(",") returns [''] (a list with one empty string), not [] (an empty list). This means warnings for empty models won't be sent as intended. Consider using value == [""] or checking value == [] or value == [""] to handle this case correctly.
| if value == "" or value == []: | |
| if ( | |
| (isinstance(value, str) and value == "") | |
| or (isinstance(value, list) and (value == [] or value == [""])) | |
| ): |
Summary
__oomol_llm_env_warnedflag to track if warning has been sentProblem
The
oomol_llm_envproperty calledsend_warning()on every access:This violates property semantics - properties should not have side effects.
Solution
Use a flag to warn only once:
Test Plan