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

Migrate to rely on GCB from GitHub actions #782

Closed
wants to merge 1 commit into from

Conversation

LukeWood
Copy link
Contributor

we unfortunately have hit the point where we cannot keep using GitHub actions due to scaling concerns. GCB allows us to configure out own CI cluster and timeouts - as such we will need to rely on that going forward.

@LukeWood
Copy link
Contributor Author

/gcbrun

1 similar comment
@LukeWood
Copy link
Contributor Author

/gcbrun

@LukeWood
Copy link
Contributor Author

/gcbrun

@bhack
Copy link
Contributor

bhack commented Sep 10, 2022

We could still use Action well known syntax but using GCP resources

https://github.com/terraform-google-modules/terraform-google-github-actions-runners

@bhack
Copy link
Contributor

bhack commented Sep 10, 2022

As a side note switching to the cloudbuild syntax will make contributing or co-maintaining the CI with the community harder.

(Just extending or make a change in the CI it will be not testable anymore locally or in a fork).

@bhack
Copy link
Contributor

bhack commented Sep 10, 2022

See also tensorflow/build#61 (comment)

@LukeWood
Copy link
Contributor Author

As a side note switching to the cloudbuild syntax will make contributing or co-maintaining the CI with the community harder.

(Just extending or make a change in the CI it will be not testable anymore locally or in a fork).

Good point, yeah this would make it much harder for community members to update it. Ideally we could still use GH-actions, so maybe self hosted is the best option.

@LukeWood
Copy link
Contributor Author

As a side note switching to the cloudbuild syntax will make contributing or co-maintaining the CI with the community harder.
(Just extending or make a change in the CI it will be not testable anymore locally or in a fork).

Good point, yeah this would make it much harder for community members to update it. Ideally we could still use GH-actions, so maybe self hosted is the best option.

As of now though… the repo is fully blocked by failing unit tests

@bhack
Copy link
Contributor

bhack commented Sep 10, 2022

As of now though… the repo is fully blocked by failing unit tests

Generally models tests are quite heavy and I don't think that we could accumulate models and run these tests on every PR. It could be ok on the PR to check the specific model when we modify a specific network and probably test all models before the nightly cut (if we can scale).

In any case I think that currently the main problem with the free hosted Action is that this test is exhausting 7GB max ram in the runner (Github free tier):
keras_cv/models/object_detection/retina_net/retina_net_inference_test.py

@qlzh727
Copy link
Member

qlzh727 commented Sep 11, 2022

If the build is failing due to heavy ram consumption, how about config some of the test to be integration test and not run them in the unit test suite?

Since the code base is expending, we should find a more scalable solution (probably discuss in the team meeting). For now, let's run the GPU test which should have the same test coverage.

Copy link
Member

@qlzh727 qlzh727 left a comment

Choose a reason for hiding this comment

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

Let's check with the team before submission.

@LukeWood
Copy link
Contributor Author

Let's check with the team before submission.

Sounds good, I also like @bhack ’s idea to disable model tests on GitHub actions and only run those before cuts

@LukeWood
Copy link
Contributor Author

Dropping this PR in favor of @bhack 's solution to just not run model tests on GH actions.

Thanks for the suggestion @bhack this is a much better suggestion!

@LukeWood LukeWood closed this Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants