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

add cirq specific environment variables to ionq #6216

Merged
merged 6 commits into from Jul 26, 2023

Conversation

splch
Copy link
Contributor

@splch splch commented Jul 22, 2023

can specify CIRQ_IONQ_API_KEY to override a global IONQ_API_KEY environment variable.

@CirqBot CirqBot added the Size: XS <10 lines changed label Jul 22, 2023
@splch splch changed the title add cirq specific environment variables add cirq specific environment variables to ionq Jul 22, 2023
Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

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

Please update unit tests for the new logic with environment variables.

Since you'd be likely changing the tests below, can you please also replace os.environ[...] = ... with mock.patch.dict(os.environ, {...}?

def test_service_api_key_via_env():
os.environ['IONQ_API_KEY'] = 'tomyheart'
service = ionq.Service(remote_host='http://example.com')
assert service.api_key == 'tomyheart'
del os.environ['IONQ_API_KEY']
def test_service_remote_host_via_env():
os.environ['IONQ_REMOTE_HOST'] = 'http://example.com'
service = ionq.Service(api_key='tomyheart')
assert service.remote_host == 'http://example.com'
del os.environ['IONQ_REMOTE_HOST']

@codecov-commenter
Copy link

codecov-commenter commented Jul 26, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01% ⚠️

Comparison is base (43f6a7f) 97.36% compared to head (05de92d) 97.36%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6216      +/-   ##
==========================================
- Coverage   97.36%   97.36%   -0.01%     
==========================================
  Files        1115     1116       +1     
  Lines       95658    95700      +42     
==========================================
+ Hits        93141    93179      +38     
- Misses       2517     2521       +4     
Files Changed Coverage Δ
cirq-google/cirq_google/engine/asyncio_executor.py 100.00% <100.00%> (ø)
cirq-google/cirq_google/engine/engine_client.py 100.00% <100.00%> (ø)
cirq-ionq/cirq_ionq/service.py 100.00% <100.00%> (ø)
cirq-ionq/cirq_ionq/service_test.py 100.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@CirqBot CirqBot added the size: S 10< lines changed <50 label Jul 26, 2023
@splch
Copy link
Contributor Author

splch commented Jul 26, 2023

Please update unit tests for the new logic with environment variables.

Since you'd be likely changing the tests below, can you please also replace os.environ[...] = ... with mock.patch.dict(os.environ, {...}?

def test_service_api_key_via_env():
os.environ['IONQ_API_KEY'] = 'tomyheart'
service = ionq.Service(remote_host='http://example.com')
assert service.api_key == 'tomyheart'
del os.environ['IONQ_API_KEY']
def test_service_remote_host_via_env():
os.environ['IONQ_REMOTE_HOST'] = 'http://example.com'
service = ionq.Service(api_key='tomyheart')
assert service.remote_host == 'http://example.com'
del os.environ['IONQ_REMOTE_HOST']

got it thanks!

service = ionq.Service(remote_host='http://example.com')
assert service.api_key == 'tomyheart'
del os.environ['IONQ_API_KEY']
with mock.patch.dict('os.environ', {'IONQ_API_KEY': 'tomyheart'}):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please pass os.environ as is instead of as a string.
For these tests you can consider decorating the test method, for example,

@mock.patch.dict(os.environ, {'IONQ_API_KEY': 'tomyheart'})
def test_service_api_key_via_env():
    service = ionq.Service(remote_host='http://example.com')
    ... 

Note if needed, you can also modify os.environ in the test function body. Any changes will be reverted when function returns and exits the patch.dict context.

Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

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

Please use os.environ object instead of "os.environ" string, it will satisfy the failing lint check.
Also, CIRQ_IONQ_REMOTE_HOST seems untested.

@splch
Copy link
Contributor Author

splch commented Jul 26, 2023

Please use os.environ object instead of "os.environ" string, it will satisfy the failing lint check. Also, CIRQ_IONQ_REMOTE_HOST seems untested.

thanks for the help :) this latest pr should cover the changes

Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you!

@pavoljuhas pavoljuhas merged commit 91f690d into quantumlib:master Jul 26, 2023
34 of 35 checks passed
@splch splch deleted the specific-cirq-api-key-and-url branch July 27, 2023 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: S 10< lines changed <50 Size: XS <10 lines changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants