Skip to content

Conversation

@anthony-yip
Copy link

Fixes #185 .

You can now just run mypy . in /opt/TangoService/Tango to check for any type errors.

Changes

  • Added an interface for all VMMS to conform to
  • Note that tashiVMMS and distDocker are currently unsupported.

…e assigned attribute), improving logging surrounding retries and giving up
…preallocator (do note that my initial worry about incorrectness was unfounded due to weird Redis sharing behavior)
@anthony-yip anthony-yip changed the base branch from master to ec2-new-implementation December 2, 2025 19:36
@coderabbitai
Copy link

coderabbitai bot commented Dec 2, 2025

Caution

Review failed

Failed to post review comments

📝 Walkthrough

Walkthrough

This PR introduces comprehensive Python 3 type annotations across the codebase while refactoring core data structures and abstractions. Changes include formalization of the VMMS interface, introduction of Redis-backed queue and dictionary abstractions, type-safe encapsulation of job and machine objects, and updates to dependencies (boto3, mypy, type stubs). The refactoring maintains backward compatibility while enabling stricter static type checking.

Changes

Cohort / File(s) Summary
Configuration & Dependencies
.gitignore, requirements.txt, Dockerfile
Updated gitignore patterns for keys and Redis; replaced boto with boto3 and added mypy with typing stubs; enhanced Docker setup with GPG key management and PYTHONPATH configuration.
Documentation & Examples
README.md, clients/job1/hello-2.sh, tests/sample_test/input/*, vmms/Dockerfile_213
Added stress testing documentation section; new shell script examples for job testing; new Dockerfile for Autolab Autograding image.
CLI & REST Interface
clients/tango-cli.py, restful_tango/server.py, restful_tango/tangoREST.py
Added RequestObj and VmObj dataclasses to tango-cli; introduced get_arg helper for safe argument access; updated tangoREST to pass instance_type, ec2_vmms, and stopBefore to TangoMachine/TangoJob; fixed import path in server.py and updated logging calls.
Core Data Structures
tangoObjects.py
Major refactor: TangoJob moved to private fields with property accessors; TangoMachine extended with instance_type and ec2_vmms; introduced TangoQueue and TangoDictionary protocols with Redis-backed (TangoRemoteQueue, TangoRemoteDictionary) and in-memory implementations (ExtendedQueue, TangoNativeDictionary).
Job & Queue Management
jobQueue.py, jobManager.py
Enhanced jobQueue with type annotations and Preallocator dependency; refactored to use new TangoDictionary and TangoQueue abstractions; updated makeDead signature to accept TangoJob objects; improved error handling and logging in jobManager with traceback context.
VM Management & Allocation
preallocator.py
Refactored to use typed Dict[str, VMMSInterface] for VMMS mapping; updated machines storage to use TangoDictionary with tuple structure (pool IDs and queue); all pool operations now use getExn for safer dictionary access.
VMMS Interface & Implementations
vmms/interface.py, vmms/sharedUtils.py, vmms/localDocker.py, vmms/ec2SSH.py, vmms/distDocker.py, vmms/tashiSSH.py
Introduced VMMSInterface protocol defining VM lifecycle methods with type hints; added VMMSUtils with shared timeout and instance name helpers; updated all VMMS backends to inherit from VMMSInterface and implement typed method signatures; ec2SSH migrated to boto3 with semaphore-based concurrency control and retry logic.
Job Execution
worker.py
Added DetachMethod enum for VM detach strategies; refactored Worker.init with type annotations; introduced afterJobExecution helper to consolidate post-job actions; added stopBefore-based early-exit checks; enhanced copyIn/runJob/copyOut calls with job.id passing.
Server Core
tango.py
Added type hints to TangoServer.init for VMMSInterface, Preallocator, and JobQueue; replaced direct timeout assignments with job.setTimeout() method calls; removed default maxOutputFileSize handling.
Build Configuration
autodriver/Makefile
Changed binary ownership from root to ubuntu while preserving setuid bit.
Testing
tests/stressTest.py, tests/testJobQueue.py, tests/testObjects.py, tests/validate.py
Added comprehensive stress testing orchestration with progress tracking, Tornado HTTP handler for result collection, and summary generation; updated existing tests to use TangoDictionary.create() and TangoQueue.create() factories; added type annotation for skip_paths variable.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Areas requiring extra attention:

  • Type consistency across tangoObjects.py refactoring: Verify that private field encapsulation with property accessors maintains backward compatibility; validate TangoJob and TangoMachine constructor signatures and usage throughout codebase
  • Data structure abstraction layer: Ensure TangoQueue and TangoDictionary factory methods properly initialize Redis vs. in-memory backends; validate getExn error handling is consistent
  • VMMS interface migration: Confirm all backends (localDocker, ec2SSH, distDocker, tashiSSH) correctly implement VMMSInterface; verify parameter passing (job_id, disableNetwork) is handled consistently
  • jobQueue.py behavioral changes: Validate makeDead(job: TangoJob) signature change and all call sites; ensure job state transitions (addToDict/deleteFromDict) preserve correctness
  • Concurrency in ec2SSH: Review semaphore acquisition/release logic and boto3 retry mechanisms; verify backward compatibility with existing EC2 workflows
  • worker.py refactoring: Validate DetachMethod enum logic paths and afterJobExecution consolidation; ensure stopBefore early-exit conditions trigger correctly
  • jobManager.py error handling: Confirm traceback logging doesn't break existing error reporting; verify preVM initialization try/except doesn't mask failures
  • Cross-file dependencies: Test interactions between jobManager → jobQueue → worker → VMMS layers given structural changes to data types and method signatures

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 31.84% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Type Annotations' is concise and directly matches the primary objective of adding type hints throughout the codebase.
Description check ✅ Passed The description references issue #185, explains the mypy checking capability, notes the new VMMS interface, and acknowledges unsupported implementations.
Linked Issues check ✅ Passed The PR successfully implements comprehensive type annotations across the codebase as required by issue #185, enabling mypy static type checking throughout the Tango project.
Out of Scope Changes check ✅ Passed All changes directly support the type annotation objective; updates to requirements.txt (mypy, type stubs), new VMMSInterface, and method signature updates are all necessary for implementing type checking.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch anthonyyip/type_annotations

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.

@KesterTan KesterTan requested review from a team and coder6583 and removed request for a team December 2, 2025 19:51
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.

Type annotations

3 participants