-
Notifications
You must be signed in to change notification settings - Fork 8
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 dependencies and migrate benchmark auth #490
Conversation
Noting some authorization methods don't require the ACCESS_KEY and SECRET_KEY.
colorama==0.4.1 # required for structlog | ||
connexion==1.4 | ||
connexion[swagger-ui]==2.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a bit weird..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll say!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me. But I am not that familiar with entity service so it might be better for Wilko to approve this PR.
@@ -24,7 +24,7 @@ | |||
import time | |||
import os | |||
|
|||
from clkhash import rest_client | |||
from anonlinkclient.rest_client import RestClient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
@@ -1,6 +1,6 @@ | |||
anonlink-client==0.0.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be 0.1.0 pretty soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet. Our depandabot will pester us when it is released :-)
@@ -232,11 +234,11 @@ def compose_result(status, tt, experiment, sizes, threshold): | |||
return result | |||
|
|||
|
|||
def delete_resources(config, credentials, run): | |||
def delete_resources(credentials, run): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be great to see what key-value pairs might be in config
in doc
rest_client.run_delete(config['server'], credentials['project_id'], run['run_id'], credentials['result_token']) | ||
rest_client.project_delete(config['server'], credentials['project_id'], credentials['result_token']) | ||
rest_client.run_delete(credentials['project_id'], run['run_id'], credentials['result_token']) | ||
rest_client.project_delete(credentials['project_id'], credentials['result_token']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it true that currently there is only server in config
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, there is lots of stuff in config
all set around line 84. the change is because the new rest_client
already knows the server address
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good. Does it still run though?
result = {} | ||
credentials = {} | ||
run = {} | ||
logger.info("Starting time: {}".format(time.asctime())) | ||
nb_parties = len(sizes) | ||
try: | ||
credentials = rest_client.project_create(server, config['schema'], 'groups', | ||
credentials = rest_client.project_create(config['schema'], 'groups', | ||
"benchy_{}".format(experiment), parties=nb_parties) | ||
# upload clks | ||
upload_binary_clks(config, sizes, credentials) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally, that should be done by the rest_client as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree => data61/anonlink-client#23
status = rest_client.wait_for_run(server, project_id, run_id, | ||
credentials['result_token'], timeout=config['timeout']) | ||
status = rest_client.wait_for_run(project_id, run_id, | ||
credentials['result_token'], timeout=config['timeout'], update_period=5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will that lead to a new line in the logs every 5 seconds? what about those huge benchmarks? How big's the log file going to be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not on the benchmark container side - we just wait until the whole thing is done (perhaps we should be outputting progress but that is another question).
On the server side we would log the request in both nginx and the flask app.
arrow | ||
boto3 | ||
clkhash==0.14.0 | ||
jsonschema | ||
pandas | ||
requests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shoule we pin all of those as well?
secretKeyRef: | ||
name: anonlink-benchmark-aws-credentials | ||
key: OBJECT_STORE_SECRET_KEY | ||
- name: OBJECT_STORE_BUCKET |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't you also have to define OBJECT_STORE_SERVER
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No we default to assuming S3 is used
This PR makes a few small changes to the benchmark container/script:
rest_client
api which.This depandabot flurry of dependency updates somehow managed to update Flask to a version that isn't compatible with our code. The integration tests passed on the PR, but not once in develop 👎
So I've also upped the version of connexion just to get the CI to pass, in theory this would be done in the PR #479 that Wilko is working on.