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 Spark 3.x support for sarplus #1566

Merged
merged 40 commits into from
Dec 14, 2021
Merged

Add Spark 3.x support for sarplus #1566

merged 40 commits into from
Dec 14, 2021

Conversation

simonzhaoms
Copy link
Collaborator

Description

Add Spark 3.x support for sarplus

Related Issues

Checklist:

  • I have followed the contribution guidelines and code style for this project.
  • I have added tests covering my contributions.
  • I have updated the documentation accordingly.
  • This PR is being made to staging branch and not to main branch.

Copy link
Collaborator

@miguelgfierro miguelgfierro left a comment

Choose a reason for hiding this comment

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

Great work!

contrib/sarplus/python/tests/test_pyspark_sar.py Outdated Show resolved Hide resolved
@@ -39,3 +39,33 @@ sbt test
```

(use ~test and it will automatically check for changes in source files, but not build.sbt)

Copy link
Collaborator

Choose a reason for hiding this comment

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

are the previous instructions still functional (e.g. sbt spPublish?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

if not I suggest removing the old instructions to avoid confusion

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, let me try to update the instructions as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated. Please take a look and leave comments if any improvements

@gramhagen
Copy link
Collaborator

does this work with the existing azureml pipeline? if not, maybe we can make an issue to create a github pipeline to build release artifacts for sarplus

@gramhagen
Copy link
Collaborator

contrib/sarplus/scala/python/setup.py Outdated Show resolved Hide resolved
@simonzhaoms
Copy link
Collaborator Author

does this work with the existing azureml pipeline? if not, maybe we can make an issue to create a github pipeline to build release artifacts for sarplus

I don't think the pipeline work with this upgrade, will update the pipeline later.

@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2021

Codecov Report

Merging #1566 (594674c) into staging (9117189) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           staging    #1566      +/-   ##
===========================================
- Coverage    61.89%   61.88%   -0.02%     
===========================================
  Files           84       84              
  Lines         8458     8458              
===========================================
- Hits          5235     5234       -1     
- Misses        3223     3224       +1     
Flag Coverage Δ
pr-gate 61.88% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
recommenders/evaluation/spark_evaluation.py 86.60% <0.00%> (-0.45%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b2b9a4...594674c. Read the comment docs.

@miguelgfierro
Copy link
Collaborator

hey @simonzhaoms in regards to:

does this work with the existing azureml pipeline? if not, maybe we can make an issue to create a github pipeline to build release artifacts for sarplus. pipeline I'm talking about is this one: https://github.com/simonzhaoms/recommenders/blob/simonz/sarplus/spark3/contrib/sarplus/azure-pipelines.yml

@laserprec is our master pipeliner, he might have some code you can reuse

@miguelgfierro
Copy link
Collaborator

btw, @simonzhaoms I noticed that you are doing the PR from a fork. I made you admin of the repo. I did the same with @angusrtaylor. Now you can create branches and do PRs from there, there is no need to fork.

@miguelgfierro
Copy link
Collaborator

@laserprec is there a way to avoid triggering the tests if there is a change in contrib or the markdown files?

In ADO, I did it like this: https://github.com/microsoft/recommenders/blob/main/tests/ci/azure_pipeline_test/dsvm_notebook_linux_cpu.yml#L10
I guess there should be a similar way in Github acctions

@laserprec
Copy link
Contributor

@laserprec is there a way to avoid triggering the tests if there is a change in contrib or the markdown files?

In ADO, I did it like this: https://github.com/microsoft/recommenders/blob/main/tests/ci/azure_pipeline_test/dsvm_notebook_linux_cpu.yml#L10 I guess there should be a similar way in Github acctions

Yes, there are similar mechanics in GitHub Action too: https://docs.github.com/en/actions/learn-github-actions/workflow-syntax-for-github-actions#example-ignoring-paths.

@anargyri
Copy link
Collaborator

anargyri commented Dec 7, 2021

@simonzhaoms I would suggest that you merge staging into this PR in order to get the latest test pipelines which should fix the failing tests.

Copy link
Collaborator

@miguelgfierro miguelgfierro left a comment

Choose a reason for hiding this comment

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

@simonzhaoms this is great, please, see my comments, the only thing I'm a little bit concerned is the token

contrib/sarplus/python/pysarplus/__init__.py Outdated Show resolved Hide resolved
contrib/sarplus/python/setup.py Outdated Show resolved Hide resolved
contrib/sarplus/python/setup.py Outdated Show resolved Hide resolved
contrib/sarplus/python/tests/test_pyspark_sar.py Outdated Show resolved Hide resolved
contrib/sarplus/scala/build.sbt Outdated Show resolved Hide resolved
contrib/sarplus/scala/build.sbt Outdated Show resolved Hide resolved
Copy link
Collaborator

@miguelgfierro miguelgfierro left a comment

Choose a reason for hiding this comment

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

this is amazing work Simon!

@anargyri anargyri merged commit 6f2088d into recommenders-team:staging Dec 14, 2021
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.

6 participants