Skip to content

Conversation

@weiguoz
Copy link
Collaborator

@weiguoz weiguoz commented Oct 23, 2019

Fix #977

@weiguoz weiguoz requested review from Yancey0623, shendiaomo, tonyyang-svail, typhoonzero and uuleon and removed request for uuleon October 23, 2019 11:05
@weiguoz weiguoz changed the title [WIP][Intermediate Representation] Add e2e test for IR analyze [Intermediate Representation] Add e2e test for IR analyze Oct 23, 2019
Yancey0623
Yancey0623 previously approved these changes Oct 23, 2019
Copy link
Collaborator

@Yancey0623 Yancey0623 left a comment

Choose a reason for hiding this comment

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

LGTM with tiny comment.

stmt := `
SELECT *
FROM housing.train
ANALYZE sqlflow_models.my_xgb_regression_model
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please note that go test may run test cases unordered, the model may be not generated when running this test, it's better to generate this model first before running ANALYZE.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The ref introduces a go test run parallel with other tests if and only if the t.Parallel() was called.

Copy link
Collaborator

@typhoonzero typhoonzero Oct 24, 2019

Choose a reason for hiding this comment

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

This is not about parallelism, it's about the order, see https://stackoverflow.com/questions/31201858/how-to-run-golang-tests-sequentially, it says:

You can't / shouldn't rely on test execution order. The order in which tests are executed is not defined, and with the use of testing flags it is possible to exclude tests from running, so you have no guarantee that they will run at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In our e2e test, the subtests are ordered by group

t.Run("CaseTrainXGBoostRegressionIR", CaseTrainXGBoostRegression)
t.Run("CasePredictXGBoostRegressionIR", CasePredictXGBoostRegression)
t.Run("CaseAnalyzeXGBoostModel", CaseTrainAndAnalyzeXGBoostModel)

But we would run go test -run CaseTrainAndAnalyzeXGBoostModel independently some times, it needs an existed model, so I add the several codes for that.

@weiguoz weiguoz merged commit b8d751a into sql-machine-learning:develop Oct 24, 2019
@weiguoz weiguoz deleted the switch_to_ir_analyze branch October 24, 2019 09:34
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.

[Intermediate Representation] Switch to the new analysis which generated by IR

3 participants