Skip to content

Ensure that failures during pyright invocation are properly surfaced in verifytypes#46418

Draft
scbedd wants to merge 3 commits intomainfrom
address-verifytypes-nonverbosity
Draft

Ensure that failures during pyright invocation are properly surfaced in verifytypes#46418
scbedd wants to merge 3 commits intomainfrom
address-verifytypes-nonverbosity

Conversation

@scbedd
Copy link
Copy Markdown
Member

@scbedd scbedd commented Apr 20, 2026

Working through a transcription failure.

Copilot AI review requested due to automatic review settings April 20, 2026 19:23
@scbedd scbedd requested a review from mccoyp as a code owner April 20, 2026 19:23
@scbedd scbedd marked this pull request as draft April 20, 2026 19:23
@scbedd scbedd self-assigned this Apr 20, 2026
@scbedd scbedd moved this from 🤔 Triage to 🔬 Dev in PR in Azure SDK EngSys 🚀🌒🧑‍🚀 Apr 20, 2026
Copy link
Copy Markdown
Contributor

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 improves the robustness of the azpysdk verifytypes flow by ensuring pyright --verifytypes failures that don’t emit valid JSON are clearly logged and treated as a failure, rather than causing an unhandled JSON parsing exception.

Changes:

  • Guard json.loads(e.output) in verifytypes to handle non-JSON/empty output and surface stdout/stderr in logs.
  • Fix incorrect logger.error(..., e) usage during dev requirements installation so the exception is properly included in the log message.
Show a summary per file
File Description
eng/tools/azure-sdk-tools/azpysdk/verifytypes.py Adds error handling for invalid/missing JSON output from pyright --verifytypes on return code 1.
eng/tools/azure-sdk-tools/azpysdk/Check.py Fixes exception logging for dev requirements installation failures.

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 1

except subprocess.CalledProcessError as e:
logger.error("Failed to install dev requirements:", e)
logger.error(f"Failed to install dev requirements: {e}")
raise e
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

In this exception handler, raise e will re-raise the exception with a new traceback and can hide where the CalledProcessError actually originated. Use a bare raise (and optionally logger.exception(...) if you want the traceback in logs) to preserve the original traceback/context.

Suggested change
raise e
raise

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

2 participants