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

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

wants to merge 2 commits into from

Conversation

krneta
Copy link
Contributor

@krneta krneta commented Oct 17, 2023

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@krneta krneta requested a review from a team as a code owner October 17, 2023 17:16
Comment on lines +23 to +33
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"}
])
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

@ajberdy
Copy link
Contributor

ajberdy commented Oct 17, 2023

Build is failing for python version validation. Looks like we need a sys.version check when creating the batch_get_image tag

],
]
})
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

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