Add benchmark upload util to Bigquery. #3776
Conversation
Also update the benchmark logger and bigquery schema for the errors found during the integration test.
This was causing error since the Kokoro test has TF_PKG=tf-nightly injected during test.
| This library require google cloud bigquery lib as dependency, which can be | ||
| installed with: | ||
| > pip install --upgrade google-cloud-bigquery |
There was a problem hiding this comment.
Also add to requirements.txt
| logging_dir: string, logging directory that contains the benchmark log. | ||
| gcp_project: string, the name of the GCP project that the log will be | ||
| uploaded to. The default project name will be detected from local | ||
| environment if no value is provide. |
| credentials: google.auth.credentials. The credential to access the | ||
| BigQuery service. The default service account credential will be | ||
| detected from local environment if no value is provided. Please use | ||
| google.oauth2.service_account.Credentials to load credential from local |
There was a problem hiding this comment.
µnit: space before "Credentials"
There was a problem hiding this comment.
google.oauth2.service_account is the module name and Credentials is the class name. Do u want me to split them with a space?
There was a problem hiding this comment.
I can't read. Ignore, and I'm sorry.
| metrics = [] | ||
| for l in lines: | ||
| if not l.strip(): continue | ||
| metric = json.loads(l) |
There was a problem hiding this comment.
I would be inclined to wrap this in a try-except (and log error). That way a single malformed line doesn't ruin all entries.
There was a problem hiding this comment.
The inline if is trying to catch the case of the last line of the file which only contains "\n". This is kind of an expected entry, maybe I should just do a inline strip and filter of the "line"?
There was a problem hiding this comment.
I don't remember the exact failure modes, but because JSON is a fragile container (1 bad byte can corrupt the entire entry), I've always seen code that skips invalid entries.
| FLAGS = None | ||
|
|
||
|
|
||
| class BigQueryUploader(object): |
There was a problem hiding this comment.
I have a conceptual proposal:
- Pull dataset_name and run_id into
__init__(). In effect make all local information part of the class state. - Support intermediate uploading. So if I call upload_metric() with 500 lines, and then call it again with 1000 the second call will upload lines 501 to 1000.
The reason is that is would allow intermediate uploading during the training loop, rather than at the very end. Depending on the sort of monitoring we want to build on top of BigTable that could be desirable. Discuss.
There was a problem hiding this comment.
Discussed offline. Since bigquery does not care about the quota, I think I can update the benchmarkLogger to contain a instance of uploader in future, and do a direct upload with writing to local file, treating bigquery as a file system.
Will address that in a separate change.
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| parser = argparse.ArgumentParser() |
There was a problem hiding this comment.
k, moving this arg_parser
| ) | ||
| parser.add_argument( | ||
| "--bigquery_data_set", "-bds", default="test_benchmark", | ||
| help="The Bigquery dataset name where the benchmark will be uploaded.", |
There was a problem hiding this comment.
How come there are no "[default: %(default)s]" for the rest of these?
| " be uploaded.", | ||
| metavar="<BMT>" | ||
| ) | ||
| FLAGS, unparsed = parser.parse_known_args() |
There was a problem hiding this comment.
Use parse_args() instead of parse_known_args() so it blows up with unrecognized args.
There was a problem hiding this comment.
Done. Moved to use arg_parser
official/utils/logging/logger.py
Outdated
| self.log_metric(key, eval_results[key], global_step=global_step) | ||
|
|
||
| def log_metric(self, name, value, unit=None, global_step=None, extras=None): | ||
| def log_metric(self, name, value, unit=None, global_step=None, extras={}): |
There was a problem hiding this comment.
Mutable types should not be default args.
|
|
||
| def setUp(self): | ||
| super(BenchmarkLoggerTest, self).setUp() | ||
| self.original_environ = dict(os.environ) |
There was a problem hiding this comment.
Could you put a comment about why this was necessary and when others would need to worry about environment variables?
4 participants
Also update the benchmark logger and bigquery schema for the
errors found during the integration test.