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

Staging to main to merge AzureML tests, improvements in SAR+ and improvements in notebooks #1730

Merged
merged 199 commits into from
Jun 6, 2022

Conversation

SahitiCheguru
Copy link
Contributor

@SahitiCheguru SahitiCheguru commented Jun 3, 2022

Description

This is a sample pull request

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.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2022

Codecov Report

Merging #1730 (53c0188) into main (d4181cf) will decrease coverage by 1.45%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1730      +/-   ##
==========================================
- Coverage   24.65%   23.20%   -1.46%     
==========================================
  Files          88       88              
  Lines        9121     9142      +21     
==========================================
- Hits         2249     2121     -128     
- Misses       6872     7021     +149     
Flag Coverage Δ
nightly 24.84% <100.00%> (+0.17%) ⬆️
pr-gate 23.20% <100.00%> (?)

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

Impacted Files Coverage Δ
recommenders/models/sar/sar_singlenode.py 97.22% <100.00%> (+0.36%) ⬆️
recommenders/datasets/mind.py 0.00% <0.00%> (-30.85%) ⬇️
recommenders/datasets/criteo.py 56.60% <0.00%> (-30.19%) ⬇️
recommenders/datasets/movielens.py 66.37% <0.00%> (-24.34%) ⬇️
recommenders/utils/spark_utils.py 84.61% <0.00%> (-11.54%) ⬇️
recommenders/datasets/download_utils.py 90.00% <0.00%> (-8.00%) ⬇️
recommenders/models/newsrec/models/base_model.py 0.00% <0.00%> (-3.02%) ⬇️
recommenders/models/newsrec/io/mind_iterator.py 0.00% <0.00%> (-0.55%) ⬇️
recommenders/utils/python_utils.py 97.50% <0.00%> (+2.50%) ⬆️

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 d4181cf...53c0188. Read the comment docs.

@miguelgfierro miguelgfierro changed the title Staging Staging to main to merge AzureML tests, improvements in SAR+ and improvements in notebooks Jun 4, 2022
@miguelgfierro
Copy link
Collaborator

@SahitiCheguru thanks for this PR, we were planning to do it after @pradnyeshjoshi has finished his work, but since you did it, we will use it.

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.

really good work

@@ -53,6 +57,8 @@ To contributors: please add your name to the list when you submit a patch to the
* **[Beth Zeranski](https://github.com/bethz)**
Copy link
Collaborator

@miguelgfierro miguelgfierro Jun 6, 2022

Choose a reason for hiding this comment

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

@pradnyeshjoshi feel free to add your name to the list of authors. This can be done in a different PR.

Comment on lines 342 to +347
# Not recommended to change this.
parser.add_argument(
"--condafile",
"--wheelfile",
action="store",
default="./reco.yaml",
help="file with environment variables",
default="./dist/recommenders-1.0.0-py3-none-any.whl",
help="recommenders whl file path",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@pradnyeshjoshi can you elaborate what you are doing here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@miguelgfierro on GitHub machines, I build a wheel from the checked out repo and wheelfile used to be the path to the wheel file (used to set up AzureML environment). The wheel file name generated by bdist command is not constant, so this is how I locate the wheel file now: https://github.com/microsoft/recommenders/blob/53c01881843b256ab33bffffa91bdf6e363e6f6a/tests/ci/azureml_tests/submit_groupwise_azureml_pytest.py#L464
I will remove the wheelfile argument in another PR since this argument is not being used anymore.

@miguelgfierro miguelgfierro merged commit aeb6b0b into main Jun 6, 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.

7 participants