Skip to content

Timer improvements - #4

Open
ShashankFC wants to merge 2 commits into
mainfrom
shashank/timer-improvements
Open

ShashankFC wants to merge 2 commits into
mainfrom
shashank/timer-improvements

Conversation

@ShashankFC

@ShashankFC ShashankFC commented Nov 24, 2025

Copy link
Copy Markdown
Collaborator

EntelligenceAI PR Summary

This PR improves timer implementation reliability and thread management in PyTorch's distributed elastic timer API.

  • Changed timestamp acquisition from time.time() to datetime.now().timestamp() in _run_watchdog() and expires() methods
  • Updated get_expired_timers() docstring to clarify expired timers use strict less-than comparison
  • Doubled watchdog thread join timeout from _max_interval to _max_interval * 2
  • Added warning log when watchdog thread fails to stop within timeout period

@entelligence-ai-pr-reviews

Copy link
Copy Markdown

Entelligence AI Vulnerability Scanner

Status: No security vulnerabilities found

Your code passed our comprehensive security analysis.

Analyzed 1 files in total

@entelligence-ai-pr-reviews

Copy link
Copy Markdown

Review Summary

🏷️ Draft Comments (3)

Skipped posting 3 draft comments that were valid but scored below your review threshold (>=13/15). Feel free to update them here.

torch/distributed/elastic/timer/api.py (3)

155-156: get_expired_timers contract changed from 'expiration_time <= deadline' to 'expiration_time < deadline', which may cause timers expiring exactly at the deadline to not be reaped, violating expected timer semantics.

📊 Impact Scores:

  • Production Impact: 4/5
  • Fix Specificity: 5/5
  • Urgency Impact: 3/5
  • Total Score: 12/15

🤖 AI Agent Prompt (Copy & Paste Ready):

In torch/distributed/elastic/timer/api.py, lines 155-156, the docstring for get_expired_timers was changed to state that a timer is expired if expiration_time < deadline, instead of expiration_time <= deadline. This breaks the contract and may cause timers that expire exactly at the deadline to not be reaped. Please revert the docstring to specify 'less than or equal to' to match expected semantics and ensure correct timer expiration.

182-188: try-except inside the watchdog loop (_watchdog_loop) causes repeated exception handling overhead on every iteration, which can significantly degrade performance under frequent error conditions.

📊 Impact Scores:

  • Production Impact: 2/5
  • Fix Specificity: 2/5
  • Urgency Impact: 1/5
  • Total Score: 5/15

🤖 AI Agent Prompt (Copy & Paste Ready):

Refactor torch/distributed/elastic/timer/api.py lines 182-188: Move the try-except block out of the main watchdog loop in `_watchdog_loop` to avoid repeated exception handling overhead. Instead, wrap the body of `_run_watchdog` in a try-except, and call a new `_run_watchdog_impl` from there. This reduces per-iteration exception handling cost and improves performance under frequent error conditions.

10-10: Unused import time increases memory footprint and can slightly slow module loading, especially in large codebases.

📊 Impact Scores:

  • Production Impact: 1/5
  • Fix Specificity: 4/5
  • Urgency Impact: 1/5
  • Total Score: 6/15

🤖 AI Agent Prompt (Copy & Paste Ready):

Remove the unused import of `time` from torch/distributed/elastic/timer/api.py at line 10 to reduce unnecessary memory usage and improve module load performance.

@entelligence-ai-pr-reviews

Copy link
Copy Markdown

Walkthrough

This PR refactors the timer implementation in PyTorch's distributed elastic module to improve timestamp handling and thread management. The primary change replaces time.time() with datetime.now().timestamp() for obtaining current timestamps in the watchdog thread and timer expiration logic. Documentation is clarified to specify that timer expiration uses strict inequality comparison. The watchdog thread shutdown mechanism is made more robust by doubling the join timeout and adding warning logs when the thread fails to terminate gracefully within the expected timeframe.

Changes

File(s) Summary
torch/distributed/elastic/timer/api.py Replaced time.time() with datetime.now().timestamp() in _run_watchdog() method and expires() context manager; updated get_expired_timers() docstring to clarify strict inequality for expiration comparison; enhanced watchdog thread shutdown by doubling join timeout to _max_interval * 2 and adding warning log for failed thread termination.

Sequence Diagram

This diagram shows the interactions between components:

sequenceDiagram
    participant Client as Client Code
    participant Expires as expires() Context
    participant TimerClient as Timer Client
    participant Queue as Request Queue
    participant Watchdog as Watchdog Thread
    participant Storage as Timer Storage
    participant Reaper as Worker Reaper

    Note over Watchdog: Watchdog Loop (_run_watchdog)
    
    loop Continuous Monitoring
        Watchdog->>Queue: get(batch_size, max_interval)
        Queue-->>Watchdog: timer_requests[]
        Watchdog->>Storage: register_timers(timer_requests)
        
        Watchdog->>Watchdog: now = datetime.now().timestamp()
        Watchdog->>Storage: get_expired_timers(now)
        Storage-->>Watchdog: expired_timers by worker_id
        
        loop For each worker with expired timers
            Watchdog->>Reaper: reap_worker(worker_id, expired_timers)
            Note over Reaper: Process expired timers<br/>for worker
        end
    end

    Note over Client,TimerClient: Countdown Timer Usage

    Client->>Expires: with expires(after=10)
    activate Expires
    Expires->>Expires: scope = caller_file#lineno
    Expires->>Expires: expiration = datetime.now().timestamp() + after
    Expires->>TimerClient: acquire(scope, expiration)
    TimerClient->>Queue: enqueue timer request
    Expires->>Client: yield (enter context)
    
    Client->>Client: Execute protected code
    
    Client->>Expires: exit context
    Expires->>TimerClient: release(scope)
    deactivate Expires

    Note over Watchdog: Improved Shutdown Sequence

    Client->>Watchdog: stop()
    activate Watchdog
    Watchdog->>Watchdog: _stop_signaled = True
    Watchdog->>Watchdog: join(max_interval * 2)
    
    alt Thread stops in time
        Watchdog->>Watchdog: Thread terminated
    else Thread still alive
        Watchdog->>Watchdog: Log warning: did not stop in time
    end
    
    Watchdog->>Watchdog: _watchdog_thread = None
    deactivate Watchdog
Loading

🔗 Cross-Repository Impact Analysis

Enable automatic detection of breaking changes across your dependent repositories. → Set up now

Learn more about Cross-Repository Analysis

What It Does

  • Automatically identifies repositories that depend on this code
  • Analyzes potential breaking changes across your entire codebase
  • Provides risk assessment before merging to prevent cross-repo issues

How to Enable

  1. Visit Settings → Code Management
  2. Configure repository dependencies
  3. Future PRs will automatically include cross-repo impact analysis!

Benefits

  • 🛡️ Prevent breaking changes across repositories
  • 🔍 Catch integration issues before they reach production
  • 📊 Better visibility into your multi-repo architecture

▶️AI Code Reviews for VS Code, Cursor, Windsurf
Install the extension

Note for Windsurf Please change the default marketplace provider to the following in the windsurf settings:

Marketplace Extension Gallery Service URL: https://marketplace.visualstudio.com/_apis/public/gallery

Marketplace Gallery Item URL: https://marketplace.visualstudio.com/items

Entelligence.ai can learn from your feedback. Simply add 👍 / 👎 emojis to teach it your preferences. More shortcuts below

Emoji Descriptions:

  • ⚠️ Potential Issue - May require further investigation.
  • 🔒 Security Vulnerability - Fix to ensure system safety.
  • 💻 Code Improvement - Suggestions to enhance code quality.
  • 🔨 Refactor Suggestion - Recommendations for restructuring code.
  • ℹ️ Others - General comments and information.

Interact with the Bot:

  • Send a message or request using the format:
    @entelligenceai + *your message*
Example: @entelligenceai Can you suggest improvements for this code?
  • Help the Bot learn by providing feedback on its responses.
    @entelligenceai + *feedback*
Example: @entelligenceai Do not comment on `save_auth` function !

Also you can trigger various commands with the bot by doing
@entelligenceai command

The current supported commands are

  1. config - shows the current config
  2. retrigger_review - retriggers the review

More commands to be added soon.

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