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

Split tests into a separate package, plus rattler-build #107

Merged
merged 13 commits into from
Dec 26, 2024

Conversation

mgorny
Copy link
Contributor

@mgorny mgorny commented Dec 24, 2024

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.
  • split tests into a separate package as suggested in Recipe improvements #22, reducing the main package size
  • removed obsolete code for CUDA < 12

Eliminate the recipe conditions for cuda_major, since only CUDA 12+ is
supported now.  Replace the jinja `{% if %}` conditions with inline
selectors, since the latter are more compatible with different tools
(e.g. conda-recipe-manager).
After moving the tests out of the main package, its size reduces
to 1.5M.  The tests package itself is 9M.

Proposed in issue conda-forge#22.
@conda-forge-admin
Copy link
Contributor

conda-forge-admin commented Dec 24, 2024

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe/meta.yaml:

  • ℹ️ The recipe is not parsable by parser conda-souschef (grayskull). This parser is not currently used by conda-forge, but may be in the future. We are collecting information to see which recipes are compatible with grayskull.
  • ℹ️ The recipe is not parsable by parser conda-recipe-manager. The recipe can only be automatically migrated to the new v1 format if it is parseable by conda-recipe-manager.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/12484953648. Examine the logs at this URL for more detail.

@hmaarrfk
Copy link
Contributor

Thanks I think reduced download sizes are definitely welcome!

recipe/meta.yaml Outdated Show resolved Hide resolved
@mgorny mgorny changed the title Split tests into a separate package, plus minor cleanup Split tests into a separate package, plus rattler-build Dec 25, 2024
@conda-forge-admin
Copy link
Contributor

conda-forge-admin commented Dec 25, 2024

Hi! This is the friendly automated conda-forge-linting service.

I failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint --conda-forge . from the recipe directory. You can also examine the workflow logs for more detail.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/12495694887. Examine the logs at this URL for more detail.

@mgorny
Copy link
Contributor Author

mgorny commented Dec 25, 2024

I've added a rattler-build conversion, and filed output request in conda-forge/admin-requests#1248. FWICS conda-smithy crashes on it, I'm going to look into that later today.

hmaarrfk pushed a commit to conda-forge/admin-requests that referenced this pull request Dec 25, 2024
@mgorny
Copy link
Contributor Author

mgorny commented Dec 25, 2024

I'm going to guess that the osx failure is a fluke.

@mgorny
Copy link
Contributor Author

mgorny commented Dec 25, 2024

The conda-smithy problems are conda-forge/conda-smithy#2165 and conda-forge/conda-smithy#2196. The former I've been able to workaround by avoiding nested conditions.

@hmaarrfk
Copy link
Contributor

I came to check in, and saw

test_decode_gif[True-treescap]

failed due to connection issues. I always feel bad since conda-forge hammers webservers which are meant to be cached locally for data.

Should we skip this test?

@hmaarrfk hmaarrfk added the automerge Merge the PR when CI passes label Dec 25, 2024
@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

  • linter: failed
  • azure: passed

Thus the PR was not passing and not merged.

@hmaarrfk hmaarrfk merged commit 50ff17c into conda-forge:main Dec 26, 2024
32 of 33 checks passed
@mgorny mgorny deleted the cleanup-and-split branch December 26, 2024 07:06
@mgorny
Copy link
Contributor Author

mgorny commented Dec 26, 2024

Should we skip this test?

Yeah, FWICS test_decode_gif goes straight for requests.get() with no caching.

@hmaarrfk
Copy link
Contributor

I guess our cis clear cache unless you work really hard not to. So any access to the internet during testing should be discouraged

@mgorny
Copy link
Contributor Author

mgorny commented Dec 26, 2024

I'm going to do a test run with pytest-socket to check for other cases of Internet access.

@mgorny
Copy link
Contributor Author

mgorny commented Dec 26, 2024

Ok, so the failures are test_decode_gif (as expected) and a few tests directly related to checking whether Internet-related API works. Should I skip the latter too, or leave them?

 │ FAILED test/test_image.py::test_decode_gif[True-gifgrid] - pytest_socket.Sock...
 │ FAILED test/test_image.py::test_decode_gif[True-fire] - pytest_socket.SocketB...
 │ FAILED test/test_image.py::test_decode_gif[True-porsche] - pytest_socket.Sock...
 │ FAILED test/test_image.py::test_decode_gif[True-treescap] - pytest_socket.Soc...
 │ FAILED test/test_image.py::test_decode_gif[True-treescap-interlaced] - pytest...
 │ FAILED test/test_image.py::test_decode_gif[True-solid2] - pytest_socket.Socke...
 │ FAILED test/test_image.py::test_decode_gif[True-x-trans] - pytest_socket.Sock...
 │ FAILED test/test_image.py::test_decode_gif[True-earth] - pytest_socket.Socket...
 │ FAILED test/test_image.py::test_decode_gif[False-gifgrid] - pytest_socket.Soc...
 │ FAILED test/test_image.py::test_decode_gif[False-fire] - pytest_socket.Socket...
 │ FAILED test/test_image.py::test_decode_gif[False-porsche] - pytest_socket.Soc...
 │ FAILED test/test_image.py::test_decode_gif[False-treescap] - pytest_socket.So...
 │ FAILED test/test_image.py::test_decode_gif[False-treescap-interlaced] - pytes...
 │ FAILED test/test_image.py::test_decode_gif[False-solid2] - pytest_socket.Sock...
 │ FAILED test/test_image.py::test_decode_gif[False-x-trans] - pytest_socket.Soc...
 │ FAILED test/test_image.py::test_decode_gif[False-earth] - pytest_socket.Socke...
 │ FAILED test/test_internet.py::TestDatasetUtils::test_download_url[True] - pyt...
 │ FAILED test/test_internet.py::TestDatasetUtils::test_download_url[False] - py...
 │ FAILED test/test_internet.py::TestDatasetUtils::test_download_url_retry_http[True]
 │ FAILED test/test_internet.py::TestDatasetUtils::test_download_url_retry_http[False]
 │ FAILED test/test_internet.py::TestDatasetUtils::test_download_url_dont_exist[True]
 │ FAILED test/test_internet.py::TestDatasetUtils::test_download_url_dont_exist[False]
 │ FAILED test/test_internet.py::TestDatasetUtils::test_download_url_dispatch_download_from_google_drive[True]
 │ FAILED test/test_internet.py::TestDatasetUtils::test_download_url_dispatch_download_from_google_drive[False]

@hmaarrfk
Copy link
Contributor

Yeah. Any test that needs a download should be skipped

google drive will definitely boot you….

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the PR when CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants