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.

Sign up for GitHub

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

Jump to bottom

Refining example, add utilities, and fix xdist test error #794

Merged
merged 31 commits into from
Nov 23, 2022
Merged

Refining example, add utilities, and fix xdist test error #794

merged 31 commits into from
Nov 23, 2022

Conversation

loomlike
Copy link
Collaborator

@loomlike loomlike commented Oct 28, 2022 •
edited
Loading

Description

This PR contains - utility functions, fix bugs, and refactor quick start (nyc taxi) notebooks.
Resolves #792 , revert #798

How was this PR tested?

Unit tests including the sample notebook test.
In a local machine, a test w/ the xdist option passed.

Does this PR introduce any user-facing changes?

  • No. You can skip the rest of this section.
  • Yes. Make sure to clarify your proposed changes.

@blrchen blrchen added the safe to test Tag to execute build pipeline for a PR from forked repo label Oct 29, 2022
blrchen
blrchen previously approved these changes Oct 29, 2022
View reviewed changes
Yuqing-cat
Yuqing-cat reviewed Oct 31, 2022
View reviewed changes
Copy link
Collaborator

@Yuqing-cat Yuqing-cat left a comment

Choose a reason for hiding this comment

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

@blrchen would you help to confirm if the test result is as expected?
databricks:====== 12 failed, 72 passed, 7 skipped, 74 warnings in 873.66s (0:14:33) =======
synapse:====== 22 failed, 63 passed, 6 skipped, 16 warnings in 1478.44s (0:24:38) ======

The latest readable CI run doesn't have that much failures: https://github.com/feathr-ai/feathr/actions/runs/3279090567/jobs/5398225684

feathr_project/feathr/client.py Show resolved Hide resolved
Yuqing-cat
Yuqing-cat previously approved these changes Oct 31, 2022
View reviewed changes
Copy link
Collaborator

@Yuqing-cat Yuqing-cat left a comment

Choose a reason for hiding this comment

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

@blrchen , let's merge it first to unblock CI testing issue. But remember to double confirm about the udf_file parameters of client.py later.

@blrchen
Copy link
Collaborator

blrchen commented Oct 31, 2022

databricks:====== 12 failed, 72 passed, 7 skipped, 74 warnings in 873.66s (0:14:33) =======
synapse:====== 22 failed, 63 passed, 6 skipped, 16 warnings in 1478.44s (0:24:38) ======

This PR only resolves the issue that test not work with pytest-xdist. For the 12 databricks failures and 22 synapse, they are related to #756. The failed test case # before this PR is before about 5 or 7)

@loomlike @Yuqing-cat i think we should revert #756 from main now for 0.9.0 release. @loomlike can submit it again to next release after all CI test failures are resolved.

@blrchen blrchen dismissed stale reviews from Yuqing-cat and themself October 31, 2022 04:53

Need more discussion

loomlike and others added 9 commits October 31, 2022 22:28
@loomlike
Revert "Revert 756 (feathr-ai#798)"
990208b 
This reverts commit ff438f5.
@loomlike
Merge branch 'jumin/fix_xdist' into jumin/revert_revert756
9c1b93e
@loomlike
revert 798 (revert756 - example notebook refactor). Also add job_util…
1fab5f2 
…s unit tests

Signed-off-by: Jun Ki Min <[email protected]>
@loomlike
Merge pull request #1 from loomlike/jumin/revert_revert756
6a4e8a1 
revert "revert756"
@blrchen @loomlike
Update test_azure_spark_e2e.py
15f4939
@blrchen @loomlike
Fix doc dead links (feathr-ai#805)
26b7a0d 
This PR fixes dead links detected in latest ci run. The doc scan ci action has been updated to run on main only, as running this in PR frequently reports false alarm due to changes in CI not deployed.
@Fendoe @loomlike
Improve UI experience and clean up ui code warnings (feathr-ai#801)
8e401b4 
* Add DataSourcesSelect and FlowGraph and ResizeTable components. Fix all warning and lint issues.

Signed-off-by: Boli Guan <[email protected]>

* Add CardDescriptions component and fix ESlint warning.

Signed-off-by: Boli Guan <[email protected]>

* Update FeatureDetails page title.

Signed-off-by: Boli Guan <[email protected]>

* Rename ProjectSelect

Signed-off-by: Boli Guan <[email protected]>

Signed-off-by: Boli Guan <[email protected]>
@blrchen @loomlike
Add release instructions for Release Candidate (feathr-ai#809)
afd9309 
* Add release instructions for Release Candidate

* Add a section for release versioning

* Add a section for overall process triggered by the release manager
@blrchen @loomlike
Bump version to 0.9.0-rc1 (feathr-ai#810)
8899f18
@loomlike
Copy link
Collaborator Author

loomlike commented Nov 1, 2022

Sorry for the large file changes. It's due to reverting #798.
Additionally, this PR includes changes in #786

@loomlike loomlike mentioned this pull request Nov 2, 2022
Merged
2 tasks
@xiaoyongzhu xiaoyongzhu reopened this Nov 3, 2022
@loomlike
fix tem file to dir
6198506 
Signed-off-by: Jun Ki Min <[email protected]>
@loomlike
Merge branch 'main' into jumin/fix_xdist
38aa149
@loomlike
checkout the feature_derivations.py from main (it was temporally chan…
ae9095c 
…ged to goaround previous issues)

Signed-off-by: Jun Ki Min <[email protected]>
@loomlike
Remove old databricks sample notebook. Change pip install feathr from…
59bd65c 
… the github main branch to pickup the latest changes always

Signed-off-by: Jun Ki Min <[email protected]>
xiaoyongzhu
xiaoyongzhu previously approved these changes Nov 14, 2022
View reviewed changes
Copy link
Member

@xiaoyongzhu xiaoyongzhu left a comment

Choose a reason for hiding this comment

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

This looks good to me. Please help fix the conflicts.

@loomlike loomlike changed the title Fix xdist test error. Also make a small cleanup some codes Refining example, add utilities, and fix xdist test error Nov 21, 2022
blrchen
blrchen previously approved these changes Nov 22, 2022
View reviewed changes
xiaoyongzhu
xiaoyongzhu previously approved these changes Nov 23, 2022
View reviewed changes
blrchen
blrchen previously approved these changes Nov 23, 2022
View reviewed changes
@loomlike loomlike dismissed stale reviews from blrchen and xiaoyongzhu via ca5d642 November 23, 2022 11:20
@xiaoyongzhu xiaoyongzhu merged commit 15550ca into feathr-ai:main Nov 23, 2022
@loomlike loomlike deleted the jumin/fix_xdist branch December 20, 2022 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blrchen blrchen blrchen approved these changes

@xiaoyongzhu xiaoyongzhu xiaoyongzhu approved these changes

@Yuqing-cat Yuqing-cat Yuqing-cat left review comments

@donegjookim donegjookim Awaiting requested review from donegjookim

Assignees
No one assigned
Labels
safe to test Tag to execute build pipeline for a PR from forked repo
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@loomlike @blrchen @xiaoyongzhu @Yuqing-cat @Fendoe