Skip to content

chore: fix model attribute accessor#11976

Merged
jordanrfrazier merged 4 commits into
mainfrom
fix-warning-model-attribute
Mar 6, 2026
Merged

chore: fix model attribute accessor#11976
jordanrfrazier merged 4 commits into
mainfrom
fix-warning-model-attribute

Conversation

@jordanrfrazier
Copy link
Copy Markdown
Collaborator

@jordanrfrazier jordanrfrazier commented Mar 2, 2026

Fixes accessor on model:

  Warning: Accessing the 'model_fields' attribute on the instance is deprecated. Instead, you should access this attribute from the model class.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 2, 2026

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: bfa36668-288f-458f-a29a-77061b430f19

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Walkthrough

This PR updates two core modules: the logging handler to properly preserve exception information in structured logs by computing messages once and passing enriched exception context through kwargs, and the cross-module schema checking to retrieve model fields from the class type rather than the instance.

Changes

Cohort / File(s) Summary
Logger Exception & Message Handling
src/lfx/src/lfx/log/logger.py
Updated remove_exception_in_production condition logic to strip exception details only when not in DEV and log level is not ERROR/CRITICAL. Modified InterceptHandler.emit to compute message once via record.getMessage(), gather exception info into kwargs, and pass both to all structlogLogger methods to preserve exception context.
Cross-Module Schema Checking
src/lfx/src/lfx/schema/cross_module.py
Changed CrossModuleMeta.__instancecheck__ to retrieve model_fields from the class type using type(instance).model_fields.keys() instead of from the instance directly.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes


Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Test Coverage For New Implementations ❌ Error No test files were added or modified to cover the code changes in logger.py and cross_module.py that modify exception handling, message processing, and isinstance checks. Add test files to verify exception logging in production, InterceptHandler message/kwargs handling, and CrossModuleMeta.model_fields instance checks.
Test Quality And Coverage ⚠️ Warning PR introduces three significant functional changes but lacks adequate test coverage for the new implementations, with critical paths modified including exc_info handling, exception preservation logic, and deprecation warning suppression. Add comprehensive tests covering actual behavioral changes: verify exc_info is passed as kwargs, test ERROR/CRITICAL exception preservation in production, and add cross_module.py deprecation warning suppression test.
Test File Naming And Structure ❓ Inconclusive Unable to verify test file patterns without actual git repository context and executed shell commands. Execute the provided shell commands in your repository to identify test files, then verify they follow your project's testing conventions.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'chore: log errors in production even with dev' directly addresses the PR's main objective of ensuring errors are logged in production, which is evident from the core changes to remove_exception_in_production logic.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Excessive Mock Usage Warning ✅ Passed Test suite demonstrates appropriate mock usage with 0.74 mocks per test, all targeting external dependencies rather than core logic.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-warning-model-attribute

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 2, 2026

Frontend Unit Test Coverage Report

Coverage Summary

Lines Statements Branches Functions
Coverage: 23%
23.24% (8235/35429) 16.02% (4458/27826) 15.91% (1185/7445)

Unit Test Results

Tests Skipped Failures Errors Time
2631 0 💤 0 ❌ 0 🔥 44.672s ⏱️

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 2, 2026

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 37.43%. Comparing base (6c4f1dd) to head (2563f30).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
src/lfx/src/lfx/schema/cross_module.py 0.00% 1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (40.00%). You can increase the patch coverage or adjust the target coverage.
❌ Your project status has failed because the head coverage (42.37%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #11976      +/-   ##
==========================================
- Coverage   37.44%   37.43%   -0.02%     
==========================================
  Files        1616     1616              
  Lines       79060    79060              
  Branches    11946    11946              
==========================================
- Hits        29607    29598       -9     
- Misses      47795    47804       +9     
  Partials     1658     1658              
Flag Coverage Δ
backend 57.34% <ø> (-0.05%) ⬇️
frontend 20.82% <ø> (ø)
lfx 42.37% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/lfx/src/lfx/schema/cross_module.py 73.33% <0.00%> (ø)

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jordanrfrazier jordanrfrazier changed the title chore: log errors in production even with dev chore: fix model attribute accessor Mar 2, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/lfx/src/lfx/schema/cross_module.py (1)

48-57: ⚠️ Potential issue | 🟠 Major

Align the guard to class-level access; check the class instead of the instance.

Line 48 checks hasattr(instance, "model_fields"), but line 57 accesses type(instance).model_fields. This inconsistency creates a robustness gap: the guard doesn't match the actual access pattern, and in edge cases could leave an unguarded class-level access. Use the class-level check to ensure consistency.

Proposed fix
-        if not hasattr(instance, "model_fields"):
+        instance_type = type(instance)
+        if not hasattr(instance_type, "model_fields"):
             return False
@@
-        instance_fields = set(type(instance).model_fields.keys())
+        instance_fields = set(instance_type.model_fields.keys())
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/lfx/src/lfx/schema/cross_module.py` around lines 48 - 57, The guard
currently checks hasattr(instance, "model_fields") but the code later accesses
type(instance).model_fields, so update the guard to perform a class-level check
(e.g., use hasattr(type(instance), "model_fields")) to match the subsequent
access; ensure the code that computes instance_fields uses the same class-level
attribute access (type(instance).model_fields) and keep the existing
cls/model_fields check for cls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/lfx/src/lfx/schema/cross_module.py`:
- Around line 48-57: The guard currently checks hasattr(instance,
"model_fields") but the code later accesses type(instance).model_fields, so
update the guard to perform a class-level check (e.g., use
hasattr(type(instance), "model_fields")) to match the subsequent access; ensure
the code that computes instance_fields uses the same class-level attribute
access (type(instance).model_fields) and keep the existing cls/model_fields
check for cls.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 355c589 and edae915.

⛔ Files ignored due to path filters (1)
  • src/frontend/package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (2)
  • src/lfx/src/lfx/log/logger.py
  • src/lfx/src/lfx/schema/cross_module.py

@github-actions github-actions Bot added the lgtm This PR has been approved by a maintainer label Mar 2, 2026
@jordanrfrazier jordanrfrazier added this pull request to the merge queue Mar 3, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Mar 3, 2026
@jordanrfrazier jordanrfrazier added this pull request to the merge queue Mar 4, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Mar 4, 2026
@jordanrfrazier jordanrfrazier added this pull request to the merge queue Mar 5, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Mar 5, 2026
@jordanrfrazier jordanrfrazier added this pull request to the merge queue Mar 6, 2026
Merged via the queue into main with commit 4fa9cd3 Mar 6, 2026
176 of 180 checks passed
@jordanrfrazier jordanrfrazier deleted the fix-warning-model-attribute branch March 6, 2026 03:01
HimavarshaVS pushed a commit that referenced this pull request Mar 10, 2026
* log errors in production even with dev; surpress model attribute warnings

* revert logger changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ignore-for-release lgtm This PR has been approved by a maintainer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants