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

Switch to pytest-lazy-fixtures #756

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

gigony
Copy link
Contributor

@gigony gigony commented Aug 5, 2024

Remove the pytest-lazy-fixture plugin and use the replacement pytest-lazy-fixtures package instead.

Address #755

@gigony gigony added bug Something isn't working improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Aug 5, 2024
@gigony gigony self-assigned this Aug 5, 2024
@gigony gigony requested review from a team as code owners August 5, 2024 18:14
@gigony gigony requested a review from bdice August 5, 2024 18:14
@gigony gigony removed the bug Something isn't working label Aug 5, 2024
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Gigon! 🙏

Had a couple questions below

@@ -1,6 +1,6 @@
The MIT License (MIT)
MIT License
Copy link
Member

Choose a reason for hiding this comment

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

Given pytest-lazy-fixtures bundles the license file in the wheel and conda packages, do we need to bundle the license file here or could it be dropped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @jakirkham !

All files under '3rdparty' folder is actually optional and we added the folder for tracking licenses internally.

Since 'LICENSE-3rdparty.md' file on the root folder covers the list of 3rdparty libraries, do you think it is better to remove whole '3rdparty' folder instead of maintaining the folder?

Copy link
Member

Choose a reason for hiding this comment

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

That's a good idea. Let's follow up after we finish the devendoring work: #753

Since some of those dependencies are statically linked now, we do need to include their licenses. Though it is possible after devendoring that we can substantially reduce or drop licenses from this list

With Python only dependencies, we could clean those up before devendoring, but it might make sense to wait until we can do a more thorough cleanup

@@ -303,7 +303,7 @@ dependencies:
- psutil>=5.8.0
- pytest>=6.2.4,<8.0.0a0
- pytest-cov>=2.12.1
- pytest-lazy-fixture>=0.6.3
- pytest-lazy-fixtures
Copy link
Member

Choose a reason for hiding this comment

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

Do we want a lower bound here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @jakirkham !
I can update it if needed.
I wonder how we can determine the lowest version to use here.

Copy link
Member

@jakirkham jakirkham Aug 5, 2024

Choose a reason for hiding this comment

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

Perhaps 1.0.0 is good enough to start?

Do the tests run ok with 1.0.0?

Suggested change
- pytest-lazy-fixtures
- pytest-lazy-fixtures>=1.0.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! Thank you! Will update this.

@jakirkham jakirkham requested a review from grlee77 August 5, 2024 20:08
@@ -16,7 +16,7 @@
import shutil

import pytest
from pytest_lazyfixture import lazy_fixture
from pytest_lazy_fixtures import lf as lazy_fixture
Copy link
Contributor

Choose a reason for hiding this comment

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

nice to see that this can continue to be used as-is without further changes to the test cases using the fixture

Remove the `pytest-lazy-fixture` plugin and use the replacement `pytest-lazy-fixtures` package instead.

Address  rapidsai#755

Signed-off-by: Gigon Bae <gbae@nvidia.com>
@gigony
Copy link
Contributor Author

gigony commented Aug 20, 2024

/merge

@rapids-bot rapids-bot bot merged commit ef99071 into rapidsai:branch-24.10 Aug 20, 2024
37 checks passed
@jakirkham jakirkham added this to the v24.10.00 milestone Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants