Skip to content

Conversation

@speediedan
Copy link
Owner

@speediedan speediedan commented Dec 6, 2025

Overview

This PR completes the migration from mypy to pyright for type checking in finetuning-scheduler. Building on the foundation established in previous work, this PR:

  1. Upgrades pyright to standard mode - Previously in basic mode, now using comprehensive type checking
  2. Removes all mypy-specific configuration - Eliminates the [tool.mypy] section and mypy-parity directives from pyproject.toml
  3. Achieves 0 errors/warnings for currently included files with pyright in standard mode
  4. Establishes patterns for expanding type checking coverage to remaining files

Changes Summary

Type Checking Fixes

Fixed Variable Scope Issues:

  • fts_supporters.py:601 - Initialize supported_keys before conditional block
  • fts_supporters.py:1201 - Move qualname initialization outside try block

Fixed Type Annotations:

  • base.py:334 - Update _module_specific_freezing parameter type to Union[torch.nn.Module, Iterable[torch.nn.Module]]
  • fsdp.py:253, 284 - Add cast(AbstractContextManager[None], ...) for context manager type inference

Fixed Conditional Imports:

  • fsdp.py - Use TYPE_CHECKING or torch.distributed.is_available() pattern for FSDP imports
  • _wrap_utils.py - Apply same pattern for consistency

Configuration Changes

pyproject.toml:

  • Upgraded typeCheckingMode from "basic" to "standard"
  • Removed all [tool.mypy] configuration section (will be done in this PR)
  • Removed mypy-parity directives (will be done in this PR)

CI/Testing

New Workflow:

  • Added .github/workflows/copilot-setup-steps.yml for remote copilot session testing
  • Reuses existing install-ci-dependencies composite action
  • Includes pyright type checking and pre-commit validation

Testing

  • pyright -p pyproject.toml passes with 0 errors, 0 warnings, 0 informations
  • ✅ All pre-commit hooks pass
  • ✅ Core functionality tests pass (11/11 tests in test_fts_explicit_implicit and test_fts_gen_ft_schedule)

Migration Strategy Checklist

This PR follows a systematic approach to complete the pyright migration:

  • ✅ Start with files that already have < 5 errors to attempt to get them back to 0
  • ✅ Continue reducing the overall type checking error count, applying pattern fixes we've identified in previous sub-issues and other patterns
  • ✅ Fix simple typing issues with minimal changes
  • ✅ Use explicit type annotations to resolve inference issues
  • ✅ Add # type: ignore for deliberate patterns (used sparingly, preferring proper type annotations)
  • ✅ Expand configuration incrementally
  • ✅ If type changes and assertions are not sufficient to address type checking errors, error on the side of being more aggressive with type checker directives rather than trying changes that could break tests and/or invalidate logic. If a true logical inconsistency is found, document it and use the appropriate type checking directive to silence the issue temporarily
  • ✅ Favor assertions with appropriate messaging over raising RuntimeError s or other exceptions
  • ✅ Maintain 100% pass rate before adding new files and before committing
  • ✅ Update pyproject.toml incrementally with your progress in reducing error counts every commit, uncomment files with 0 errors that can newly be included in pyright checks

Patterns Established

1. Conditional Import Pattern

from typing import TYPE_CHECKING

if TYPE_CHECKING or torch.distributed.is_available():
    from torch.distributed.fsdp import FullyShardedDataParallel

2. Context Manager Type Casting

from contextlib import AbstractContextManagerfrom typing import castwith cast(AbstractContextManager[None], _get_full_state_dict_context(...)):    # context manager body

3. Variable Initialization Before Conditionals

# Initialize with sensible default
supported_keys = set(config.keys())
if self.automatic_optimization:
    # Override with proper value in conditional
    supported_keys = {field.name for field in fields(LRSchedulerConfig)}
# Now safe to use outside conditional
return {k: v for k, v in config.items() if k in supported_keys}

📚 Documentation preview 📚: https://finetuning-scheduler--23.org.readthedocs.build/en/23/

- Add `requirements/utils/prune_torch_deps.py`:
  - Implements iterative, transitive pruning of packages that are only required by `torch` (e.g., `networkx`, `sympy`, `mpmath`).
  - Provides a CLI: `python prune_torch_deps.py <lockfile_path>`.
  - Parses uv lockfile `# via` comments and prunes exclusive transitive deps using safe iterations.

- Simplify `requirements/utils/lock_ci_requirements.sh`:
  - Remove complex awk-based pruning logic and replace it with a simple call:
    `python "${SCRIPT_DIR}/prune_torch_deps.py" "${lockfile}"`.
  - Keep nightly lockfile generation behavior and the two-step manual install workflow.
  - Use `--index` (replaces deprecated `--extra-index-url`) for PyTorch nightly resolution.

- Security & maintenance:
  - Document security rationale: restrict the dependency confusion attack surface by pruning torch-only packages while using `--index-strategy unsafe-best-match` during lockfile generation only.
  - Keep installation workflow secure: manual two-step install or explicit nightly index for runtime.

- Tests & checks:
  - All unit tests pass (164 passed, 52 skipped).
  - Pre-commit hooks pass.
  - Module placed in `requirements/utils/` — not included in `src/` coverage.

Files changed:
- Added: `requirements/utils/prune_torch_deps.py`
- Updated: `requirements/utils/lock_ci_requirements.sh`
- Updated: `requirements/ci/requirements.txt` and `requirements/ci/requirements-oldest.txt` (torch-only deps pruned)

Notes:
- This refactor improves maintainability and makes pruning logic easier to test and extend.
- No change to the user-install workflow; nightly torch is still installed separately as before.
…set pyright mode to standard and remove initial warnings
@speediedan
Copy link
Owner Author

speediedan commented Dec 6, 2025

@copilot
All tests are passing initially. Starting with these existing commits follow the systematic approach discussed in this description to complete the pyright migration, removing all mypy parity pyproject.toml custom configuration and fixing the type issues that arise using this checklist:

✅ Continue reducing the overall type checking error count, applying pattern fixes we've identified in the description and previous commits along with idiomatic patterns
✅ Fix simple typing issues with minimal changes
✅ Use explicit type annotations to resolve inference issues
✅ Add # type: ignore for deliberate patterns (used sparingly, preferring proper type annotations)
✅ Expand configuration incrementally
✅ If type changes and assertions are not sufficient to address type checking errors, error on the side of being more aggressive with type checker directives rather than trying changes that could break tests and/or invalidate logic. If a true logical inconsistency is found, document it and use the appropriate type checking directive to silence the issue temporarily
✅ Favor assertions with appropriate messaging over raising RuntimeError s or other exceptions
✅ Maintain 100% pass rate (with cpu tests) before adding new files and before committing
✅ Update pyproject.toml incrementally with your progress in reducing error counts every commit, uncomment files with 0 errors that can newly be included in pyright checks (the existing exclude list of directories can remain as is, other files may need to be temporarily added)

Copy link
Contributor

Copilot AI commented Dec 6, 2025

@speediedan I've opened a new pull request, #24, to work on those changes. Once the pull request is ready, I'll request review from you.

* Initial plan

* Remove mypy configuration and update pyright comments

Co-authored-by: speediedan <[email protected]>

* Enable reportAssignmentType check and fix variable shadowing in _reduce_transition

Co-authored-by: speediedan <[email protected]>

* Enable reportCallIssue check and fix LR scheduler instantiation

Co-authored-by: speediedan <[email protected]>

* Enable reportIndexIssue check and fix sharded optimizer indexing

Co-authored-by: speediedan <[email protected]>

* Enable reportGeneralTypeIssues and reorganize pyright config comments

Co-authored-by: speediedan <[email protected]>

* Fix spelling: Recieved -> Received in error messages

Co-authored-by: speediedan <[email protected]>

---------

Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: speediedan <[email protected]>
@speediedan speediedan marked this pull request as ready for review December 6, 2025 22:00
Copilot AI review requested due to automatic review settings December 6, 2025 22:00
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR successfully completes the migration from mypy to pyright for type checking in the finetuning-scheduler repository. The migration upgrades pyright from "basic" to "standard" mode and removes all mypy-specific configuration. The PR achieves 0 type checking errors/warnings by applying targeted type annotation fixes and establishes reusable patterns for future type checking work.

Key changes include:

  • Upgraded pyright to standard mode with comprehensive type checking enabled
  • Fixed type inference issues through explicit type annotations, context manager casting, and variable scope adjustments
  • Added security-focused dependency pruning utility to minimize attack surface when using PyTorch nightly
  • Corrected multiple spelling errors in error messages ("Recieved" → "Received")

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
pyproject.toml Upgraded pyright to standard mode, removed mypy configuration, enabled comprehensive type checking rules
src/finetuning_scheduler/strategy_adapters/base.py Updated _module_specific_freezing parameter type to accept both single Module and Iterable of Modules
src/finetuning_scheduler/strategy_adapters/fsdp.py Added TYPE_CHECKING conditional import pattern and AbstractContextManager casting for context managers
src/finetuning_scheduler/strategy_adapters/_wrap_utils.py Applied TYPE_CHECKING conditional import pattern for consistency with fsdp.py
src/finetuning_scheduler/fts_supporters.py Fixed variable scope issues, corrected spelling errors, added type ignore directives for unavoidable inference limitations
src/finetuning_scheduler/fts.py Renamed variables for type clarity to avoid shadowing
requirements/utils/prune_torch_deps.py New utility script to prune torch-only dependencies from lockfiles to reduce dependency confusion attack surface
requirements/utils/lock_ci_requirements.sh Integrated torch dependency pruning, updated security documentation, changed --extra-index-url to --index
requirements/ci/requirements.txt Removed torch-only dependencies (mpmath, networkx, sympy), updated fastapi and protobuf versions
requirements/ci/requirements-oldest.txt Updated fastapi and protobuf versions to match main requirements
.github/workflows/copilot-setup-steps.yml New CI workflow for testing copilot environment setup with pyright validation

@speediedan speediedan merged commit b07a271 into main Dec 6, 2025
16 checks passed
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.

2 participants