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: LocalQuantumTask to use ExecutionManager #657

Conversation

Sai-prakash15
Copy link
Contributor

@Sai-prakash15 Sai-prakash15 commented Aug 3, 2023

Issue #, if available:

Description of changes:

Testing done:

Merge Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.

General

Tests

  • I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • I have checked that my tests are not configured for a specific region or account (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@Sai-prakash15 Sai-prakash15 requested a review from a team as a code owner August 3, 2023 23:55
@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Merging #657 (637ef80) into feature/execute-manager-local-quantum-task (da00284) will not change coverage.
Report is 6 commits behind head on feature/execute-manager-local-quantum-task.
The diff coverage is 100.00%.

@@                             Coverage Diff                              @@
##           feature/execute-manager-local-quantum-task      #657   +/-   ##
============================================================================
  Coverage                                      100.00%   100.00%           
============================================================================
  Files                                             121       122    +1     
  Lines                                            7891      7982   +91     
  Branches                                         1753      1770   +17     
============================================================================
+ Hits                                             7891      7982   +91     
Files Changed Coverage Δ
src/braket/aws/aws_quantum_task_batch.py 100.00% <100.00%> (ø)
src/braket/devices/local_simulator.py 100.00% <100.00%> (ø)
src/braket/tasks/local_quantum_task.py 100.00% <100.00%> (ø)
src/braket/tasks/local_quantum_task_batch.py 100.00% <100.00%> (ø)
src/braket/tasks/quantum_task_helper.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

Comment on lines 74 to 76
if hasattr(self, "_execute_manager"):
return self._execute_manager.cancel()
raise NotImplementedError("LocalQuantumTask does not support cancelling")
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using hasattr, you can just do an if self._execute_manager check, and set it to None if not supplied

Copy link
Contributor

Choose a reason for hiding this comment

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

getattr(self, "_execute_manager", None)?

src/braket/devices/local_simulator.py Outdated Show resolved Hide resolved
@Sai-prakash15 Sai-prakash15 changed the title feat: LocalQuantumTask to use ExecuteManager feat: LocalQuantumTask to use ExecutionManager Aug 8, 2023
Copy link
Member

@speller26 speller26 left a comment

Choose a reason for hiding this comment

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

Looking good, just minor stuff

src/braket/tasks/local_quantum_task.py Outdated Show resolved Hide resolved
"""
task = LocalQuantumTask()
task._execution_manager = execution_manager
task.async_result()
Copy link
Member

Choose a reason for hiding this comment

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

The task should be started without having to call async_result

@Sai-prakash15 Sai-prakash15 force-pushed the local-quantum-task-using-execute-manager branch from d515025 to a98648c Compare August 15, 2023 18:41
setup.py Outdated
@@ -28,7 +28,7 @@
package_dir={"": "src"},
install_requires=[
"amazon-braket-schemas>=1.18.0",
"amazon-braket-default-simulator>=1.19.0",
"amazon-braket-default-simulator @ git+https://github.com/aws/amazon-braket-default-simulator-python.git@task-executor", # noqa
Copy link
Member

Choose a reason for hiding this comment

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

Once you've merged the default sim changes, remember to change this branch

Suggested change
"amazon-braket-default-simulator @ git+https://github.com/aws/amazon-braket-default-simulator-python.git@task-executor", # noqa
"amazon-braket-default-simulator @ git+https://github.com/aws/amazon-braket-default-simulator-python.git@feature/execute-manager", # noqa

tox.ini Outdated
@@ -95,4 +95,4 @@ commands =
deps =
# If you need to test on a certain branch, add @<branch-name> after .git
git+https://github.com/aws/amazon-braket-schemas-python.git
git+https://github.com/aws/amazon-braket-default-simulator-python.git
git+https://github.com/aws/amazon-braket-default-simulator-python.git@task-executor
Copy link
Member

Choose a reason for hiding this comment

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

Here too

Suggested change
git+https://github.com/aws/amazon-braket-default-simulator-python.git@task-executor
git+https://github.com/aws/amazon-braket-default-simulator-python.git@feature/execute-manager

@Sai-prakash15 Sai-prakash15 merged commit a7302cf into feature/execute-manager-local-quantum-task Aug 17, 2023
16 of 28 checks passed
@Sai-prakash15 Sai-prakash15 deleted the local-quantum-task-using-execute-manager branch August 17, 2023 21:21
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.

3 participants