refactor: unnest task runs, have parent id instead#1176
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRefactors TaskRun to remove nested TaskRun-as-parent support: TaskRuns are now always children of Tasks and may reference a parent run via a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the TaskRun data model to use a flat storage structure instead of a nested file system hierarchy. TaskRun objects now reside directly under their parent Task and maintain lineage through a new parent_task_run_id field. This change simplifies the codebase by removing complex polymorphic parent logic, custom parent loading overrides, and expensive recursive file system traversals. Feedback was provided regarding a potential performance regression in parent type validation during child iteration, suggesting an optimization to avoid full model loading.
📊 Coverage ReportOverall Coverage: 91% Diff: origin/main...HEAD
Summary
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/core/kiln_ai/adapters/model_adapters/base_adapter.py`:
- Around line 609-612: The code is using parent_task_run.id as
parent_task_run_id even when parent_task_run may be unsaved (TaskRun.id can be
cleared later), which risks dangling references; modify the block that sets
parent=self.task and parent_task_run_id to first verify parent_task_run is saved
(e.g., check parent_task_run is not None and parent_task_run.id is not None) and
if it is unsaved either raise a clear exception (ValueError) instructing callers
to persist the parent TaskRun first or explicitly persist/save the
parent_task_run before reading its id; ensure you reference and guard the
parent_task_run and parent_task_run_id usage in the same method so no transient
IDs are passed through.
In `@libs/core/kiln_ai/datamodel/basemodel.py`:
- Around line 614-616: The code currently calls
cls.parent_type().load_from_file(parent_path) unconditionally which will try to
open a directory when parent_path is a folder; first check whether parent_path
is a directory (os.path.isdir(parent_path)) and if so handle it before calling
load_from_file: if the parent type provides a directory loader (e.g., a
load_from_dir or similar on cls.parent_type()), call that with parent_path;
otherwise locate a concrete file inside the directory (e.g., the first matching
child file) and pass that file path into
cls.parent_type().load_from_file(parent_file_path); ensure the variable names
referenced are parent_path, cls.parent_type(), and load_from_file so the change
is easy to find.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4be709ad-9f92-4c04-a320-2e66bec504f6
📒 Files selected for processing (9)
app/web_ui/src/lib/api_schema.d.tslibs/core/kiln_ai/adapters/model_adapters/base_adapter.pylibs/core/kiln_ai/adapters/model_adapters/test_saving_adapter_results.pylibs/core/kiln_ai/datamodel/basemodel.pylibs/core/kiln_ai/datamodel/task.pylibs/core/kiln_ai/datamodel/task_run.pylibs/core/kiln_ai/datamodel/test_basemodel.pylibs/core/kiln_ai/datamodel/test_models.pylibs/core/kiln_ai/datamodel/test_task.py
💤 Files with no reviewable changes (1)
- libs/core/kiln_ai/datamodel/task.py
| default=None, | ||
| description="The trace of the task run in OpenAI format. This is the list of messages that were sent to/from the model.", | ||
| ) | ||
| parent_task_run_id: str | None = Field( |
There was a problem hiding this comment.
This is the only real addition in this class in this PR, the rest is undoing what had been added in #1126
| return None | ||
| return self.parent # type: ignore | ||
|
|
||
| def find_task_run_by_id_dfs( |
There was a problem hiding this comment.
Was added in #1126, no longer needed since we flatten all TaskRuns and don't nest them in directory structures now
| self, | ||
| expected_parent_types: List[Type[KilnBaseModel]] | None = None, | ||
| ) -> Self: | ||
| @model_validator(mode="after") |
There was a problem hiding this comment.
This file diff only undoes what was added in #1126
|
Do we need to do anything for existing task runs that have embedded task runs? |
There are none right now - that was not being used by anybody (except me for multiturn chat) |
2 participants
What does this PR do?
Rollback #1126 that was nesting TaskRuns into each other through our parenting system. Instead, we do it through having a parent id on the task run, which will simplify.
Checklists
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests