Skip to content

Conversation

qlzh727
Copy link
Member

@qlzh727 qlzh727 commented May 8, 2018

  1. All three type of benchmark loggers are behind the control flag:
    benchmark_logger_type.

  2. The benchmark_uploader is spilt into lib and main module.

  3. Added test cases with mock for bigquery related classes.

@qlzh727 qlzh727 requested review from a team and karmel May 8, 2018 22:09
@qlzh727 qlzh727 force-pushed the bigquery-logger branch from d0f2ef8 to 44bb280 Compare May 8, 2018 22:49
@qlzh727 qlzh727 requested a review from MarkDaoust as a code owner May 8, 2018 22:49
1. All three type of benchmark loggers are behind the control flag:
benchmark_logger_type.

2. The benchmark_uploader is spilt into lib and main module.

3. Added test cases with mock for bigquery related classes.

Move the benchmark_uploader back to restore the change delta.

Readd the benchmark_uploader.

Move the benchmark_logger to new location.
@qlzh727 qlzh727 force-pushed the bigquery-logger branch from 44bb280 to 27df433 Compare May 8, 2018 22:54
@karmel
Copy link
Contributor

karmel commented May 8, 2018

Hmm, it is hard to review this because of the move; what changed in the moved file? For future reference, there is a git mv that I think helps git track changes a little more clearly.

"colab": {
"name": "eager.ipynb",
"version": "0.3.2",
"views": {},
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 a whole bunch of changes got pulled in... I will hold off reviewing until the changes are resolved, just in case the solution is, "delete this PR and start over."

@qlzh727
Copy link
Member Author

qlzh727 commented May 9, 2018

Closing this PR and created new one #4210

@qlzh727 qlzh727 closed this May 9, 2018
@qlzh727 qlzh727 deleted the bigquery-logger branch May 11, 2018 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants