mirror of
https://github.com/Significant-Gravitas/AutoGPT.git
synced 2026-04-08 03:00:28 -04:00
chore: remove accidentally committed file
This commit is contained in:
@@ -1,144 +0,0 @@
|
||||
# Nick's Review - Outstanding Items for Discussion
|
||||
|
||||
**PR #11699 - Dynamic LLM Registry**
|
||||
|
||||
---
|
||||
|
||||
## 🔴 CRITICAL (Merge Blockers)
|
||||
|
||||
### 1. Thundering Herd Problem
|
||||
**Location:** Overall architecture
|
||||
**Issue:** At scale (100 pods x 10 threads = 1000+ simultaneous DB locks on registry refresh)
|
||||
**Nick's suggestion:** Use Redis cache - one service writes, everyone reads from cache
|
||||
**Options:**
|
||||
- A) Full Redis cache layer (preferred by Nick)
|
||||
- B) Single-writer pattern with Redis lock (quicker interim)
|
||||
- C) Rate limiting/jitter (band-aid)
|
||||
|
||||
**Status:** Not started
|
||||
**Estimate:** 6-8 hours for full Redis cache
|
||||
|
||||
---
|
||||
|
||||
## 🟠 DESIGN QUESTIONS (Need Decisions)
|
||||
|
||||
### 2. Route Organization
|
||||
**Location:** `api/features/admin/llm_routes.py`
|
||||
**Issue:** Routes are under `/admin/llm` but models are shared between directories
|
||||
**Nick's question:** Should routes be under `/llm` instead of `/admin` since models aren't admin-specific?
|
||||
**Current:** `/api/llm/admin` (admin) and `/api/llm` (public)
|
||||
**Status:** Already fixed routing, but organizational question remains
|
||||
|
||||
### 3. Disabled Models in Validation
|
||||
**Location:** `data/llm_registry/schema_utils.py`
|
||||
**Issue:** Current design allows disabled models to pass validation, runtime fallback handles it
|
||||
**Nick's question:** "is that a good user experience?"
|
||||
**Options:**
|
||||
- Permissive (current): Old graphs with disabled models still work, runtime fallback
|
||||
- Strict: Fail fast if disabled model selected, force user to update
|
||||
**Status:** Design decision needed - depends on migration UX
|
||||
|
||||
### 4. Dedicated Registry Worker
|
||||
**Location:** `api/ws_api.py`
|
||||
**Issue:** Dedicated async worker constantly waiting for rare events (registry updates every few days)
|
||||
**Nick's suggestion:** Generic "refresh page" notification system that could be reused for updates, registry changes, etc.
|
||||
**Status:** Bigger refactor - defer or propose alternative?
|
||||
|
||||
### 5. Try-Catch Pattern Consistency
|
||||
**Location:** `api/features/admin/llm_routes.py` lines 417-425
|
||||
**Issue:** Extensive try-catch blocks wrapping every route
|
||||
**Nick's question:** "are all these try catches actually helping us? does this follow the existing pattern?"
|
||||
**Action:** Check existing route patterns in codebase to match style
|
||||
**Status:** Need investigation
|
||||
|
||||
---
|
||||
|
||||
## 🟡 CODE QUALITY (Should Fix)
|
||||
|
||||
### 6. Dangerous Continue on Error
|
||||
**Location:** `data/block_cost_config.py` lines 112-115
|
||||
```python
|
||||
logger.warning("Failed to query model migration overrides: %s. Proceeding with default costs.", exc)
|
||||
```
|
||||
**Nick's concern:** "this seems like a dangerous place to continue by default"
|
||||
**Action:** Should probably raise or at least make it more obvious this is a fallback
|
||||
**Status:** Easy fix - 5 minutes
|
||||
|
||||
### 7. Backwards Compat Re-exports
|
||||
**Location:** `data/llm_registry/__init__.py` lines 10-15
|
||||
```python
|
||||
# Re-export for backwards compatibility
|
||||
from backend.data.llm_registry.notifications import (...)
|
||||
```
|
||||
**Nick's question:** "why? what backwards compatibility are we providing? update the whole system to support it"
|
||||
**Action:** Check if these are actually used elsewhere or just cruft
|
||||
**Status:** Need to grep codebase for usage
|
||||
|
||||
### 8. Module-Level Re-exports
|
||||
**Location:** `data/llm_registry/__init__.py` lines 16-40
|
||||
**Issue:** Large block of re-exports from sub-modules
|
||||
**Nick's question:** "do you actually import from the module level or is this an autogenerated file we don't use and will have to upkeep?"
|
||||
**Action:** Check if anything imports from `backend.data.llm_registry` vs direct sub-modules
|
||||
**Status:** Need to grep codebase for import patterns
|
||||
|
||||
### 9. _json_to_dict Util Duplication
|
||||
**Location:** `data/llm_registry/registry.py`
|
||||
**Nick's question:** "is this not a util already? also if its prisma json it should probably take that in as the type"
|
||||
**Action:** Check if existing util exists, consolidate if possible
|
||||
**Status:** Quick fix if util exists - 10 minutes
|
||||
|
||||
### 10. Dataclasses vs Pydantic
|
||||
**Location:** `data/llm_registry/registry.py`
|
||||
**Issue:** Registry uses `@dataclass` instead of Pydantic models
|
||||
**Nick's request:** "these should be pydantic base models or a justification as to why not"
|
||||
**Status:** Bigger refactor - need justification or migrate
|
||||
**Estimate:** 1-2 hours to migrate, or provide justification for dataclasses
|
||||
|
||||
### 11. operation_id Change
|
||||
**Location:** `api/features/store/routes.py`
|
||||
```python
|
||||
operation_id="getV2GetCreatorDetails"
|
||||
```
|
||||
**Nick's question:** "why this change?"
|
||||
**Action:** Explain if needed for OpenAPI collision fix, or remove if not
|
||||
**Status:** Quick check - 5 minutes
|
||||
|
||||
### 12. o-series Detection
|
||||
**Location:** `blocks/llm.py`
|
||||
**Issue:** Hardcoded regex `r"(^|/)o\d"` to detect o-series models
|
||||
**Nick's request:** Should come from DB model metadata, not hardcoded pattern
|
||||
**Action:** Add `supportsParallelToolCalls` field or similar to model schema
|
||||
**Status:** Medium refactor - 1 hour
|
||||
|
||||
---
|
||||
|
||||
## ✅ FIXED TODAY
|
||||
|
||||
1. ✅ Inline imports moved to top-level (execution_analytics_routes.py, manager.py, graph.py)
|
||||
2. ✅ Empty state UX message removed
|
||||
3. ✅ gpt-4o hardcode → DEFAULT_LLM_MODEL env var
|
||||
4. ✅ Route prefix narrowed from `/api` to `/api/llm`
|
||||
5. ✅ Removed unnecessary import alias
|
||||
6. ✅ Capabilities migrated from Provider to Model
|
||||
7. ✅ Removed commented code and obvious comments in llm.py
|
||||
|
||||
---
|
||||
|
||||
## SUMMARY
|
||||
|
||||
**Must discuss:**
|
||||
- Thundering herd solution approach (Redis cache vs alternatives)
|
||||
- Route organization philosophy
|
||||
- Disabled model validation UX strategy
|
||||
- Dedicated worker vs generic notification system
|
||||
|
||||
**Quick wins to do after call:**
|
||||
- Dangerous continue on error (#6)
|
||||
- Check backwards compat re-exports usage (#7, #8)
|
||||
- operation_id justification (#11)
|
||||
|
||||
**Bigger refactors to decide:**
|
||||
- Dataclasses → Pydantic (#10)
|
||||
- o-series detection from DB (#12)
|
||||
- _json_to_dict consolidation (#9)
|
||||
- Try-catch pattern review (#5)
|
||||
Reference in New Issue
Block a user