Skip to content

refactor: remove 'run' optional dependency group#471

Merged
DoganK01 merged 1 commit intomasterfrom
update-env
Nov 13, 2025
Merged

refactor: remove 'run' optional dependency group#471
DoganK01 merged 1 commit intomasterfrom
update-env

Conversation

@DoganK01
Copy link
Copy Markdown
Member

  • Remove "run" optional dependency group

@claude
Copy link
Copy Markdown

claude bot commented Nov 13, 2025

Code Review: Remove 'run' Optional Dependency Group

Summary

This PR removes the deprecated 'run' optional dependency group from pyproject.toml. After thorough review of the codebase, this change appears to be a valid refactoring.


✅ Positive Observations

1. Code Quality & Best Practices

  • Clean refactoring that removes duplicate dependencies
  • The removed dependencies (fastapi, python-multipart) are already present in the 'web' optional dependency group
  • Follows proper semantic versioning with a 'refactor:' commit prefix
  • Minimal, focused change (1 addition, 35 deletions)

2. Codebase Analysis

  • The CLI's run_command() function in src/upsonic/cli/commands.py uses lazy imports for FastAPI dependencies (lines 15-32), which gracefully handles missing dependencies
  • When dependencies are missing, it provides a helpful error message: "FastAPI dependencies not found. Please run: upsonic install" (line 663)
  • No references to --extra run or [run] found in documentation, README, or CI workflows
  • The 'web' group already includes the necessary dependencies for the run command

3. Backward Compatibility

  • The lazy import pattern ensures the CLI won't break for users who don't need FastAPI functionality
  • Users who previously used uv sync --extra run can now use uv sync --extra web instead
  • The upsonic init command generates config files with dependencies that include fastapi/uvicorn, so users can still get these via upsonic install

⚠️ Concerns & Recommendations

1. Documentation & Migration Path

  • Issue: No migration guide or documentation update included
  • Recommendation: Consider adding a note to the README or creating a CHANGELOG entry mentioning:
    • That the 'run' extra has been removed
    • Users should use 'web' extra instead
    • Example: uv sync --extra web instead of uv sync --extra run

2. Potential User Impact

  • Issue: Existing users or scripts using uv sync --extra run will fail silently (uv will likely ignore the unknown extra)
  • Recommendation: Consider whether a deprecation warning in the previous release would have been helpful
  • Note: Since this is likely a relatively new feature and the 'web' group provides the same functionality, the impact should be minimal

3. Test Coverage

  • Observation: No test changes in this PR
  • Question: Are there any integration tests that specifically test uv sync --extra run? If so, they should be updated to use --extra web
  • Recommendation: Verify that CI tests still pass with the 'web' extra

4. Consistency Check

  • Question: Why does the 'web' group include celery>=5.5.3 and uvicorn>=0.35.0 but not fastapi?
  • Context: The CLI's run_command needs FastAPI, yet the 'web' group doesn't explicitly list it
  • Finding: After checking the codebase, I see that upsonic init generates a config file that includes fastapi in the "api" dependencies section, which users install via upsonic install. This is a different installation pattern than I initially expected.

🔒 Security Considerations

  • No security concerns identified
  • Dependency versions remain unchanged for existing packages
  • No new dependencies introduced

🎯 Performance Considerations

  • Slightly reduces dependency metadata size
  • No runtime performance impact
  • May slightly speed up uv lock operations due to fewer dependency groups

📋 Suggested Actions

Before Merging:

  1. ✅ Verify all CI tests pass
  2. ⚠️ Consider adding a brief note in README or docs about the change
  3. ⚠️ Search for any scripts or documentation (internal or external) that reference --extra run
  4. ✅ Confirm that upsonic run command still works when dependencies are installed via 'web' extra

After Merging:

  1. Monitor for user reports of installation issues
  2. Update any external documentation or blog posts that mention the 'run' extra

📊 Overall Assessment

Verdict: ✅ Approve with minor suggestions

This is a clean refactoring that removes duplication. The main risk is existing users or documentation referencing the old 'run' extra, but the codebase's lazy import pattern provides good protection against breakage.

Code Quality: 5/5
Test Coverage: N/A (no tests needed for dependency removal)
Documentation: 3/5 (could use migration notes)
Security: 5/5
Performance: 5/5


🤖 Review Details

@DoganK01 DoganK01 merged commit abf8801 into master Nov 13, 2025
4 checks passed
@DoganK01 DoganK01 deleted the update-env branch November 13, 2025 17:18
@claude claude bot mentioned this pull request Nov 13, 2025
DoganK01 added a commit that referenced this pull request Jan 5, 2026
refactor: remove 'run' optional dependency group
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant