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.

Sign up for GitHub

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

fix getting started for jobs. #418

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix getting started for jobs. #418

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9db4bf5
fix getting started for jobs.
krneta Oct 17, 2023
47f8c13
fix image tag per python version
krneta Oct 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions ...b/Creating_your_first_Hybrid_Job_mocks.py → ...0_Creating_your_first_Hybrid_Job_mocks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,17 @@ def pre_run_inject(mock_utils):
]
}
])
mocker.set_create_job_result({
"jobArn" : f"arn:aws:braket:{mocker.region_name}:000000:job/testJob"
})
if mocker.mock_level == 'ALL':
# All full mocking is done in us-west-2
mocker.set_create_job_result({
"jobArn" : f"arn:aws:braket:{mocker.region_name}:000000:job/testJob"
})
else:
# When running on QPU, the job arn is based on the device arn region, even when mocking.
mocker.set_create_job_side_effect([
{ "jobArn" : f"arn:aws:braket:{mocker.region_name}:000000:job/testJob"},
{ "jobArn" : f"arn:aws:braket:us-west-1:000000:job/testJob"}
])
Comment on lines +24 to +34
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need a branch here? Why not just always mock the second way? (I think I was getting errors when I was trying that, so I believe there's an answer, I just don't know it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's the reason. I tried to add those comments to explain why it's different between the two ways of mocking. We could change the way we do that, or we can add this branch here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the reason the second mocking doesn't work for all mocking is because AwsSession.copy_session(region) doesn't create a mock in the proper region. I've isolated this issue down to boto3.Session(region_name=new_region) not doing anything with region_name. Can we update the wrapper to fix this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it was because the all mocking wrapper always returns us-west-2: https://github.com/amazon-braket/amazon-braket-examples/blob/main/test/integ_tests/mock_utils.py#L174

No?

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, though because of where the region was being pulled from, the specific line of code was a few lines up

mocker.set_get_job_result({
"instanceConfig" : {
"instanceCount" : 1
Expand All @@ -36,6 +44,19 @@ def pre_run_inject(mock_utils):
mocker.set_log_streams_result({
"logStreams": []
})
mocker.set_start_query_result({
"queryId": "TestId"
})
mocker.set_get_query_results_result({
"status": "Complete",
"results": [
[
{"field": "@message", "value": "iteration_number=0;expval=10;"},
{"field": "@timestamp", "value": "0"}
],
]
})
mocker.set_batch_get_image_result({"images": [{"imageId": {"imageDigest": "my-digest", "imageTag": "-py310-"}}]})
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this needs to depend on the currently running python version

global default_job_results
default_job_results = mock_utils.read_file("../job_results.json", __file__)
with open("results.json", "w") as f:
Expand Down
6 changes: 6 additions & 0 deletions test/integ_tests/mock_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ def set_search_result(self, result):
def set_create_job_result(self, result):
self._wrapper.boto_client.create_job.return_value = result

def set_create_job_side_effect(self, side_effect):
self._wrapper.boto_client.create_job.side_effect = side_effect

def set_get_job_result(self, result):
self._wrapper.boto_client.get_job.return_value = result

Expand All @@ -62,6 +65,9 @@ def set_get_query_results_result(self, result):
def set_list_objects_v2_result(self, result):
self._wrapper.boto_client.list_objects_v2.return_value = result

def set_batch_get_image_result(self, result):
self._wrapper.boto_client.batch_get_image.return_value = result

@property
def region_name(self):
return self._wrapper.region_name
Expand Down
1 change: 0 additions & 1 deletion test/integ_tests/test_all_notebooks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
]
# Python 3.10 required for decorators notebooks
EXCLUDED_NOTEBOOKS += [
"0_Creating_your_first_Hybrid_Job.ipynb",
"Quantum_machine_learning_in_Amazon_Braket_Hybrid_Jobs.ipynb",
"Using_PennyLane_with_Braket_Hybrid_Jobs.ipynb",
"0_Getting_started.ipynb",
Expand Down
Loading