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 TF MNIST classification cost benchmark #33391

Merged
merged 6 commits into from
Dec 17, 2024
Merged

Add TF MNIST classification cost benchmark #33391

merged 6 commits into from
Dec 17, 2024

Conversation

jrmccluskey
Copy link
Contributor

Adds the Tensorflow MNIST classification example as a dataflow cost benchmark workflow as a second example for benchmark implementation. Has the added wrinkle of additional dependencies that need to be installed on workers, unlike the wordcount example.


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests
Go tests

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

Copy link
Contributor

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @shunping for label python.
R: @damccorm for label build.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

Copy link
Contributor

@damccorm damccorm left a comment

Choose a reason for hiding this comment

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

Just had one comment, otherwise LGTM

-PloadTest.mainClass=apache_beam.testing.benchmarks.inference.tensorflow_mnist_classification_cost_benchmark \
-Prunner=DataflowRunner \
-PpythonVersion=3.10 \
'-PloadTest.args=${{ env.beam_Inference_Python_Benchmarks_Dataflow_test_arguments_1 }} --job_name=benchmark-tests-tf-mnist-classification-python-${{env.NOW_UTC}} --input_file=gs://apache-beam-ml/testing/inputs/it_mnist_data.csv --output_file=gs://temp-storage-for-end-to-end-tests/wordcount/result_tf_mnist-${{env.NOW_UTC}}.txt --model=gs://apache-beam-ml/models/tensorflow/mnist/' \
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider running multiple benchmarks in the same workflow instead of a workflow per test? The advantage would be having fewer things to monitor/maintain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eventually we can bundle them either by framework or put them all together in one workflow, this is largely just me building on the wordcount example benchmark by having a RunInference-specific instance (the most important distinction is the need to include a requirements file for Dataflow workers, but the pattern will largely hold for custom containers with CUDA deps too.)

If we wanted to go ahead and choose one of those routes we could go ahead and do that now + set the workflow up for cron scheduling, I'm not opposed to that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be in favor of just doing that now - I think a single workflow will end up being easier to manage, and we can always parallelize via jobs within the workflow if needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

easy enough, done

Copy link
Contributor

@damccorm damccorm left a comment

Choose a reason for hiding this comment

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

Thanks!

@jrmccluskey
Copy link
Contributor Author

Build wheel failure is unrelated, merging

@jrmccluskey jrmccluskey merged commit 0e37501 into master Dec 17, 2024
89 of 94 checks passed
Polber pushed a commit to Polber/beam that referenced this pull request Dec 17, 2024
* Add TF MNIST classification cost benchmark

* linting

* Generalize to single workflow file for cost benchmarks

* fix incorrect UTC time in comment

* move wordcount to same workflow

* update workflow job name
Polber pushed a commit to Polber/beam that referenced this pull request Dec 17, 2024
* Add TF MNIST classification cost benchmark

* linting

* Generalize to single workflow file for cost benchmarks

* fix incorrect UTC time in comment

* move wordcount to same workflow

* update workflow job name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants