Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Re-enable AutoML model create tests #3721

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

Closed
wants to merge 5 commits into from
Closed

Conversation

busunkim96
Copy link
Contributor

@busunkim96 busunkim96 commented May 11, 2020

Closes #3706.

I don't have the context around why these tests were disabled. Does these tests need to be modified in some other way?

#1608 disabled these tests.

@busunkim96 busunkim96 requested a review from sirtorry May 11, 2020 17:28
@busunkim96 busunkim96 requested review from nnegrey, telpirion and a team as code owners May 11, 2020 17:28
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 11, 2020
@tmatsuo
Copy link
Contributor

tmatsuo commented May 11, 2020

Few things.

  1. The tests are using the following code:
model_name = "test_" + datetime.datetime.now().strftime("%Y%m%d%H%M%S")

This easily conflicts among multiple builds. Can you update this to use uuid?

  1. The tests are calceling the operation, but it's a best effort trial of cancellation, so sometimes the models are created before the cancellation happens. I think we should try to delete the models in teardown code.

translate/automl/model_test.py Show resolved Hide resolved
@sirtorry
Copy link
Contributor

these samples are outdated. samples and tests can be deleted. translate doc and vision doc reference other snippets.

@tmatsuo
Copy link
Contributor

tmatsuo commented May 15, 2020

@sirtorry
Thanks for the input.

I did a quick search for the region tag, and there are still used in multiple location. Although they're all translated page, can we really delete them now?

@sirtorry
Copy link
Contributor

I'm going to submit a request for these samples to be replaced on our localised/non-english documentation. We shouldn't remove the samples until that's been done. In this case, should we re-enable these tests?

@busunkim96
Copy link
Contributor Author

Opened #3766 to track removing those samples.

I'm alright with closing this and leaving the tests disabled. @tmatsuo Is that alright with you?

@tmatsuo
Copy link
Contributor

tmatsuo commented May 15, 2020

@busunkim96 Yes, it's alright!

@busunkim96 busunkim96 closed this May 15, 2020
@anguillanneuf anguillanneuf deleted the automl-reenable branch November 3, 2021 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-enable model test
4 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.