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

BUG in new tests #1995

Merged
merged 10 commits into from
Sep 22, 2023
Merged

BUG in new tests #1995

merged 10 commits into from
Sep 22, 2023

Conversation

miguelgfierro
Copy link
Collaborator

@miguelgfierro miguelgfierro commented Sep 19, 2023

Description

Related Issues

Failed run: https://github.com/recommenders-team/recommenders/actions/runs/6239152244

References

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.

Signed-off-by: miguelgfierro <miguelgfierro@users.noreply.github.com>
Signed-off-by: miguelgfierro <miguelgfierro@users.noreply.github.com>
Signed-off-by: miguelgfierro <miguelgfierro@users.noreply.github.com>
Signed-off-by: miguelgfierro <miguelgfierro@users.noreply.github.com>
Signed-off-by: miguelgfierro <miguelgfierro@users.noreply.github.com>
@anargyri
Copy link
Collaborator

We still have some references to Microsoft in the text we should review in another PR e.g. https://github.com/recommenders-team/recommenders/blob/bug/new_tests/SECURITY.md#:~:text=Security-,Microsoft,-takes%20the%20security

@anargyri
Copy link
Collaborator

Also in setup.py the Microsoft references need to be fixed.
And another PR to regenerate the documentations page.

@anargyri
Copy link
Collaborator

I guess also contributing license is not relevant any more and should be replaced with something from LF.

Signed-off-by: miguelgfierro <miguelgfierro@users.noreply.github.com>
Signed-off-by: miguelgfierro <miguelgfierro@users.noreply.github.com>
Signed-off-by: miguelgfierro <miguelgfierro@users.noreply.github.com>
@miguelgfierro
Copy link
Collaborator Author

I updated your comments @anargyri. I´m not sure about the new email given by the LF, I´ve tried to write them to see how it works.

@@ -68,8 +68,3 @@ Try to be empathic.

</details>

## Microsoft Contributor License Agreement
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ibrahimhaddad Does LF require some similar type of agreement for contributors?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tracked here: #1997

},
author="RecoDev Team at Microsoft",
author="Recommenders contributors",
author_email="RecoDevTeam@service.microsoft.com",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Email too is outdated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have sent an email to Ibrahim to see which one we can add

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tracked here: #1997

@@ -1,35 +0,0 @@
<!-- BEGIN MICROSOFT SECURITY.MD V0.0.1 BLOCK -->
Copy link
Collaborator

@anargyri anargyri Sep 20, 2023

Choose a reason for hiding this comment

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

Also @ibrahimhaddad do we need to include anything about LF security policy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tracked here: #1997

@loomlike
Copy link
Collaborator

We still have some references to Microsoft in the text we should review in another PR e.g. https://github.com/recommenders-team/recommenders/blob/bug/new_tests/SECURITY.md#:~:text=Security-,Microsoft,-takes%20the%20security

can you create an issue for that to track? or did you already created one?

@@ -261,6 +261,7 @@ def test_geoimc_functional(notebooks, output_notebook, kernel_name, expected_val

@pytest.mark.notebooks
@pytest.mark.experimental
@pytest.mark.skip(reason="xLearn pip package has installation incompatibilities")
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this temporal? or do we want to skip only at some condition, like using skipif("some_module" not in sys.modules, reason="'some_module' was not installed correctly maybe due to version conflict")?

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 reality, it is a matter than someone has time to integrate xlearn in the core libraries

Copy link
Collaborator

@loomlike loomlike left a comment

Choose a reason for hiding this comment

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

Thanks. I left a comment regarding skipping a test for your reference.

Signed-off-by: miguelgfierro <miguelgfierro@users.noreply.github.com>
@miguelgfierro
Copy link
Collaborator Author

can you create an issue for that to track? or did you already created one?

Tracked here: #1997

@miguelgfierro miguelgfierro merged commit 6a2b0a3 into staging Sep 22, 2023
20 checks passed
@miguelgfierro miguelgfierro deleted the bug/new_tests branch September 22, 2023 18:29
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.

None yet

3 participants