Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEAT] Handle batches with partial results #707

Merged
merged 10 commits into from
Sep 18, 2024
Merged

Conversation

MatthieuMoreau0
Copy link
Collaborator

  • Stop raising an Exception if one of the job of the batch has no results after fetching.
  • Stop raising an exception when fetching results if the status of the batch is a termination status.

Currently if one of the job of a batch fails, it is impossible to retrieve the results for any jobs of the batch - this raises an exception. In this PR, I suggest instead to add None to the list of results for that submission when a job has no results. I don't know if returning None is the most idiomatic way to do this in pulser; I also contemplated creating a new subclass of Result called EmptyResult but I wanted to gather some feedback before going for that because it would imply some refactoring of the Result class and I'm not entirely convinced it would bring much.

@MatthieuMoreau0 MatthieuMoreau0 self-assigned this Jul 10, 2024
@MatthieuMoreau0 MatthieuMoreau0 marked this pull request as draft July 10, 2024 10:44
@HGSilveri
Copy link
Collaborator

HGSilveri commented Jul 10, 2024

I don't know if returning None is the most idiomatic way to do this in pulser; I also contemplated creating a new subclass of Result called EmptyResult but I wanted to gather some feedback before going for that because it would imply some refactoring of the Result class and I'm not entirely convinced it would bring much.

I was thinking of something along those lines too, I think it would be better than None. I also considered having PendingResult for distinguishing pending jobs from failed ones but that's maybe beyond the scope of this PR.

@HGSilveri
Copy link
Collaborator

I think we both agree that the current format that we use to return results is limiting us and that there doesn't seem to be a nice way to include partial results within it.
As such, I would propose we add an alternative, more flexible method to RemoteResults instead. I was thinking of something along the lines of

RemoteResults.get_available_results() -> dict[JobId, Result]:

This would give the user a workaround when something failed in the traditional method. We could also point the user to this alternative method through the error message.
What do you think about this @MatthieuMoreau0 ?

@HGSilveri HGSilveri added this to the v0.20 Release milestone Sep 11, 2024
Copy link
Collaborator

@HGSilveri HGSilveri left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this!

pulser-pasqal/pulser_pasqal/pasqal_cloud.py Outdated Show resolved Hide resolved
pulser-core/pulser/backend/remote.py Outdated Show resolved Hide resolved
pulser-core/pulser/backend/remote.py Outdated Show resolved Hide resolved
pulser-core/pulser/backend/remote.py Outdated Show resolved Hide resolved
pulser-core/pulser/backend/remote.py Outdated Show resolved Hide resolved
pulser-pasqal/pulser_pasqal/pasqal_cloud.py Outdated Show resolved Hide resolved
pulser-pasqal/pulser_pasqal/pasqal_cloud.py Show resolved Hide resolved
pulser-core/pulser/backend/remote.py Outdated Show resolved Hide resolved
pulser-pasqal/pulser_pasqal/pasqal_cloud.py Outdated Show resolved Hide resolved
pulser-core/pulser/backend/remote.py Outdated Show resolved Hide resolved
MatthieuMoreau0 and others added 2 commits September 18, 2024 11:12
Co-authored-by: Henrique Silvério <29920212+HGSilveri@users.noreply.github.com>
Copy link
Collaborator

@HGSilveri HGSilveri left a comment

Choose a reason for hiding this comment

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

I think I got why you needed so many changes in the tests but let me know if I got it wrong

tests/test_pasqal.py Outdated Show resolved Hide resolved
tests/test_pasqal.py Show resolved Hide resolved
tests/test_pasqal.py Outdated Show resolved Hide resolved
tests/test_pasqal.py Outdated Show resolved Hide resolved
tests/test_pasqal.py Show resolved Hide resolved
@MatthieuMoreau0 MatthieuMoreau0 marked this pull request as ready for review September 18, 2024 15:56
Copy link
Collaborator

@HGSilveri HGSilveri left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot for your work on this!

@HGSilveri HGSilveri merged commit 7af1d2d into develop Sep 18, 2024
9 checks passed
@HGSilveri HGSilveri mentioned this pull request Sep 20, 2024
HGSilveri added a commit that referenced this pull request Sep 20, 2024
**Main changes:**
- Reworking the NoiseModel interface (#710)
- Allow modification of the EOM setpoint without disabling EOM mode (#708)
- Enable definition of effective noise operators in all basis (#716)
- Add leakage (#720)
- Support differentiability through Torch tensors (#703)
- Add from_abstract_repr to Device and VirtualDevice (#727) 
- [FEAT] Handle batches with partial results (#707)
- Add open batches to pulser-pasqal (#701)
@HGSilveri HGSilveri deleted the mm/partial-results branch November 12, 2024 11:18
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