Skip to content

[PASQAL] Add pasqal clients retry policy - #155

Open
sarjevane wants to merge 6 commits into
qiskit-community:mainfrom
sarjevane:main
Open

[PASQAL] Add pasqal clients retry policy#155
sarjevane wants to merge 6 commits into
qiskit-community:mainfrom
sarjevane:main

Conversation

@sarjevane

Copy link
Copy Markdown
Collaborator

Description of Change

  • Add PASQAL's pasqal_common package containing common retry logic
  • Apply retry policy for both pasqal-local and pasqal-cloud
  • Disable retry policy using environment variable (for e2e tests) using QRMI_PASQAL_RETRIES_DISABLED=1
  • Handle two classes of retries:
    • rate-limiting (423/429) with handling of Retry-After, defaults to jitter / exponential backoff, max 5 minutes
    • transient errors (5xx) - with jitter and exponential backoff, max 2 minutes

Checklist ✅

  • Have you included a description of this change?
  • Have you updated the relevant documentation to reflect this change?
  • Have you made sure CI is passing before requesting a review?

@CLAassistant

CLAassistant commented Jul 3, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@badtst badtst left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, I'd suggest adapting the way we match against the QRMI_PASQAL_RETRIES_DISABLED env variable for usage with the qrmi_config.json

Comment thread dependencies/pasqal_common/src/retry.rs Outdated
Comment thread dependencies/pasqal_common/src/retry.rs Outdated
Comment thread dependencies/pasqal_common/src/retry.rs
Comment thread src/pasqal.rs Outdated
@sarjevane
sarjevane requested a review from badtst July 6, 2026 15:19

@badtst badtst left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lgtm ! Waiting for feedback on the variable name

Comment thread src/pasqal.rs
/// `<backend_name>_QRMI_PASQAL_RETRIES_DISABLED` (e.g.
/// `PASQAL_LOCAL_QRMI_PASQAL_RETRIES_DISABLED`), falling back to the unprefixed
/// name as a global override (e.g. for e2e tests).
const RETRIES_DISABLED_ENV: &str = "QRMI_PASQAL_RETRIES_DISABLED";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

An opinion on the naming @MatthieuMoreau0 @awennersteen ?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

An alternative suggestion: instead of activating/disabling retries with a boolean env variable, have you considered using this to define the maximum number of retries? It can be set to 0 to disable retries but can also be tweaked for finer controls over the retries

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I like Matthieus suggestion.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I had not considered the amount of retries, only the time budget, given the initial use-case - retry in case of transient errors to the Cloud.

IMHO a time budget (as a single value) is easier to reason with than an initial delay + exponential backoff + number of retries, so we could make the time-budget configurable (instead of simple on/off switch) - that would be my preference.

We can otherwise configure the number of retries, but that alone is only half of the story, we probably would need to make the initial delay configurable - but that feels like over-engineering to me, most users will not tweak these much.

Happy to discuss this further offline

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We now have a unified Middleware, which uses retries. If the number of retries is set to 0, then there is no retries (also in case of 429)

Comment thread dependencies/pasqal_common/src/retry.rs
Comment thread dependencies/pasqal_common/src/retry.rs Outdated
@sarjevane
sarjevane marked this pull request as draft July 13, 2026 09:38
@sarjevane sarjevane closed this Jul 15, 2026
@sarjevane sarjevane reopened this Jul 15, 2026
@sarjevane
sarjevane marked this pull request as ready for review July 15, 2026 15:12
@badtst
badtst self-requested a review July 15, 2026 15:16
@sarjevane

sarjevane commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator Author

The only failure is a timeout on the schedmd/slurm website which is down at the moment - https://github.com/qiskit-community/qrmi/actions/runs/29724719180#summary-88297102759 - unrelated to the changes of this PR.


Edit: schedmd's website is back up, re-running the failed test fixed it.

@sarjevane sarjevane self-assigned this Jul 20, 2026
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.

5 participants