test: add decorator test #122
Conversation
| @@ -4,6 +4,7 @@ boto3==1.28.53 | |||
| certifi>=2023.7.22 | |||
| invoke==2.2.0 | |||
| mock | |||
| pennylane | |||
There was a problem hiding this comment.
Added because it is used in the entry point script. Not necessarily a new constraint from decorator jobs-- we'd need to do this if the entry point was a function for legacy jobs too (vs a file)
There was a problem hiding this comment.
Where is pennylane used in the entry point script? Do you mean the container entry point script or the actual customer script below? In either case, it shouldn't be required for running the test (pennylane will be included in the container itself).
There was a problem hiding this comment.
Pennylane is globally imported in the file defining the customer entry point function, meaning it needs to be installed when we import the function. We can prevent this by importing PL within the job scope (inside the entry point function), but that would be messier and involve refactoring some of our helpers. In practice it's incredibly unlikely imo that customers are writing PL jobs without having PL locally.
The reason we didn't need this previously is because we were using a whole file as an entry point script (__main__ method). Even with the old interface, if we had instead chosen a specific function to use for entry point, we would have run into this same issue.
|
Failing with access denied exception after rebasing latest changes from main (succeeded before) |
| @@ -44,7 +44,7 @@ def record_test_metrics(metric, start_time, interface): | |||
| 'Unit': 'Seconds', | |||
| 'Value': time.time() - start_time | |||
| }], | |||
| Namespace='braket-container-metrics' | |||
| Namespace='/aws/braket' | |||
There was a problem hiding this comment.
Can you change the type to "braket_container_tests" so we know what the metrics relate to? Or maybe we need a new dimension to make it clear this is not a regular aws braket service metric.
There was a problem hiding this comment.
Have you verified this got published to the right place?
There was a problem hiding this comment.
Working as expected!
There was a problem hiding this comment.
OK, we'll need to update the dashboard CDK as well
| from ..common.braket_jobs_util import job_test | ||
|
|
||
|
|
||
| def test_qaoa_circuit(account, role, s3_bucket, image_list): | ||
| @pytest.mark.parametrize("decorator", (False, True)) |
There was a problem hiding this comment.
I think it's overkill to test with and without decorators when we're just trying to see if the containers are valid.
| @@ -158,7 +150,3 @@ def cost_function(params): | |||
|
|
|||
| record_test_metrics('Total', start_time, pl_interface) | |||
| print("Braket Container Run Success") | |||
|
|
|||
|
|
|||
| if __name__ == "__main__": | |||
There was a problem hiding this comment.
We're no longer running this module as a script, only importing entry_point from here. We only needed this before cause the script itself got run as "main".
Issue #, if available:
Description of changes:
Run braket test as decorator
Testing done:
Test passes
Merge Checklist
Put an
xin 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
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.