-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Fixed SAR notebooks #1738
Fixed SAR notebooks #1738
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChuyangKe congrats for your first PR! Awesome!
@pradnyeshjoshi this PR has an error with the AzureML credentials, same as #1736, however, this one doesn't have any error: #1739 Do you know what could be happening? |
@miguelgfierro Looks like @ChuyangKe has created a fork of the repo, and GitHub Actions has no way to make secrets available to forks, so it's not able to find the login credentials. |
@pradnyeshjoshi Yes I created a fork to my account. Do I need to push in other ways? |
@ChuyangKe we usually create a feature branch off of staging in the same repo and create a pull request into staging. AzureML tests in this PR are failing because the credentials to log into AzureML (as part of unit test workflow) are stored as repo level secrets. I am looking into how we can support PRs from forks. |
@pradnyeshjoshi the same thing is failing here: #1732 are you sure it is related to the fork? |
@ChuyangKe I have added you to the repo, from now on, you don't have to create a fork, you just need to create a branch from staging and then PR to staging |
@miguelgfierro In #1732, the error occurs because the group name mentioned in the workflow file does not exist anymore in the |
I'm rerunning again all the tests FYI @pradnyeshjoshi @ChuyangKe |
I will merge this because I think with the new PR of @pradnyeshjoshi solved the AzureML issue |
Description
Fixed the error that in SAR notebooks, the TOP_K parameter is not properly supplied to function model.recommend_k_items().
Related Issues
Checklist:
staging branch
and not tomain branch
.