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

Skip tests using Internet, enable AVIF/HEIF #108

Merged
merged 13 commits into from
Dec 27, 2024

Conversation

mgorny
Copy link
Contributor

@mgorny mgorny commented Dec 26, 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.

Skipped the tests that were using Internet, and added pytest-socket to ensure that Internet access is blocked and if anything tries to use it in the future, it'll fail and let us know.

Should I bump build number, given that nothing is changed in the installed package? No clue if we can merge it without actually building new packages.

@conda-forge-admin
Copy link
Contributor

conda-forge-admin commented Dec 26, 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/12512160860. Examine the logs at this URL for more detail.

@hmaarrfk
Copy link
Contributor

The previous build fully passed. I think it is "bad practice" to not bump the build number.

I had a question on the name of the build_number variable.

For conda recipes (i.e. not rattle), we had a bug where the bot would only bump the number if it was called build.

  • number would not work.
  • I'm not sure about build_number.

I'm not even sure that we can automatically bump rattler recipes with the infrastructure just yet.

Copy link
Contributor

@hmaarrfk hmaarrfk left a comment

Choose a reason for hiding this comment

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

Just have a small question about variable names

@conda-forge-admin please rerender

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

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

@mgorny
Copy link
Contributor Author

mgorny commented Dec 26, 2024

Well, I've used build_number because I've seen that specifically mentioned in the context of rattler-build, but it was just an example, so maybe it doesn't matter. FWICS the WIP pull request supports any variable name starting with build:

https://github.com/regro/cf-scripts/pull/2920/files#diff-64cf0236b90d1d20a9b344b82d67dae576dc9df854e0f32ba14f0895058d3bc1R16-R23

@hmaarrfk hmaarrfk mentioned this pull request Dec 26, 2024
10 tasks
@mgorny mgorny changed the title Skip tests using Internet Skip tests using Internet, enable AVIF/HEIF Dec 26, 2024
@mgorny
Copy link
Contributor Author

mgorny commented Dec 26, 2024

Enable AVIF and HEIF, confirmed the libraries get linked and some AVIF/"HEIC" tests pass.

@hmaarrfk
Copy link
Contributor

@conda-forge-admin please rerender

@hmaarrfk
Copy link
Contributor

(ps. you have to rerender enthusiastically when you add new dependencies)

conda-forge-webservices[bot] and others added 2 commits December 26, 2024 17:12
@hmaarrfk
Copy link
Contributor

Actually, we should also add libheif to the global pinnings https://github.com/conda-forge/libheif-feedstock/blob/main/recipe/meta.yaml#L21

I typically wait till "more than 1 package" depends on something..

@mgorny
Copy link
Contributor Author

mgorny commented Dec 26, 2024

Actually, we should also add libheif to the global pinnings https://github.com/conda-forge/libheif-feedstock/blob/main/recipe/meta.yaml#L21

I typically wait till "more than 1 package" depends on something..

Do you want me to submit a pull request?

@hmaarrfk
Copy link
Contributor

Do you want me to submit a pull request?

Sure i'm focusd on my own technical project right now:

  1. I typically create a "migration" to the latest version.
  2. Within the migration yaml i add a comment as a todo (this is super easy to miss at the end)

see for example
https://github.com/conda-forge/conda-forge-pinning-feedstock/pull/6804/files

@hmaarrfk
Copy link
Contributor

(How much are these "tutorials" useful to you. I just haven't seen you around conda-forge prior to pytorch so I'm hoping some of this is well received)

@mgorny
Copy link
Contributor Author

mgorny commented Dec 26, 2024

(How much are these "tutorials" useful to you. I just haven't seen you around conda-forge prior to pytorch so I'm hoping some of this is well received)

It's my second month with Conda, so some hints are always helpful. Thank you!

mgorny added a commit to mgorny/conda-forge-pinning-feedstock that referenced this pull request Dec 26, 2024
@hmaarrfk
Copy link
Contributor

@conda-forge-admin please rerender

I manually added the migration file and hopefully the pin will take.

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

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

@hmaarrfk
Copy link
Contributor

I'm not sure why the new pinning didn't take, it might be that a new version of the pinnings package needs to be fully released (i.e. propagate through the cloud)

@hmaarrfk
Copy link
Contributor

@conda-forge-admin please rerender

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

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

@hmaarrfk
Copy link
Contributor

Honestly, so glad we started the migrator. I didn't even predict: conda-forge/libheif-feedstock#29

@mgorny
Copy link
Contributor Author

mgorny commented Dec 27, 2024

Yeah, it's a fun coincidence. "Nobody expects the Spanish inquisition", I guess.

Comment on lines +72 to +73
${{ '2024/12 These tests use Internet' if 0 }}
or test_decode_gif or test_download_url or "test_get_model[lraspp"
Copy link
Member

Choose a reason for hiding this comment

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

Really not a fan of this style of commenting...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Me neither. I've filed prefix-dev/rattler-build#1289 for what I believe would be the best way of solving the problem.

Comment on lines 213 to 218
- if: not aarch64
then: pytest --verbose -k "not (${{ tests_to_skip }})" --durations=50 test/
then: pytest --disable-socket --verbose -k "not (${{ tests_to_skip }})" --durations=50 test/
- if: aarch64 and (build_platform == target_platform)
then: pytest -k "not (${{ tests_to_skip }})" --durations=50 test/
then: pytest --disable-socket -k "not (${{ tests_to_skip }})" --durations=50 test/
- if: aarch64 and (build_platform != target_platform)
then: true
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for separate invocations here? Just the --verbose?
Why not

        script:
          - if: not aarch64 or (build_platform == target_platform)
            then: pytest --disable-socket --verbose -k "not (${{ tests_to_skip }})" --durations=50 test/
          # nothing else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I have no clue why. I mean — it was like that.

"Nothing else" is not an option since if/then is handled by preprocessor, and then aarch64 and (build_platform != target_platform) yields an invalid empty script:.

That said, I'm not sure if we really need the condition in the first place. rattler-build may have improved the support for testing via emulator, so I'll retry that later today.

Copy link
Member

Choose a reason for hiding this comment

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

Let's clean this up with the next PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, either I'm doing something wrong or the build_platform != target_platform condition doesn't seem to work at all. I'm investigating right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, it's the other (i.e. the python: group) tests failing, and they're failing both with rattler-build and with the old conda-build recipe (tested as of 477803a).

recipe/build.sh Outdated Show resolved Hide resolved
Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

OK, let's get this in, thanks!

Comment on lines 213 to 218
- if: not aarch64
then: pytest --verbose -k "not (${{ tests_to_skip }})" --durations=50 test/
then: pytest --disable-socket --verbose -k "not (${{ tests_to_skip }})" --durations=50 test/
- if: aarch64 and (build_platform == target_platform)
then: pytest -k "not (${{ tests_to_skip }})" --durations=50 test/
then: pytest --disable-socket -k "not (${{ tests_to_skip }})" --durations=50 test/
- if: aarch64 and (build_platform != target_platform)
then: true
Copy link
Member

Choose a reason for hiding this comment

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

Let's clean this up with the next PR.

@h-vetinari h-vetinari merged commit 2b88f0c into conda-forge:main Dec 27, 2024
32 of 33 checks passed
@mgorny mgorny deleted the skip-net-tests branch December 27, 2024 11:13
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.

4 participants