-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
Conversation
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 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. |
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 For conda recipes (i.e. not rattle), we had a bug where the bot would only bump the number if it was called
I'm not even sure that we can automatically bump rattler recipes with the infrastructure just yet. |
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.
Just have a small question about variable names
@conda-forge-admin please rerender
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. |
Well, I've used |
Enable AVIF and HEIF, confirmed the libraries get linked and some AVIF/"HEIC" tests pass. |
@conda-forge-admin please rerender |
(ps. you have to rerender enthusiastically when you add new dependencies) |
…onda-forge-pinning 2024.12.26.03.25.00
Actually, we should also add I typically wait till "more than 1 package" depends on something.. |
Do you want me to submit a pull request? |
Sure i'm focusd on my own technical project right now:
see for example |
(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! |
Suggested by @hmaarrfk in conda-forge/torchvision-feedstock#108. Thanks!
@conda-forge-admin please rerender I manually added the migration file and hopefully the pin will take. |
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. |
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) |
@conda-forge-admin please rerender |
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. |
…onda-forge-pinning 2024.12.26.18.39.45
Honestly, so glad we started the migrator. I didn't even predict: conda-forge/libheif-feedstock#29 |
Yeah, it's a fun coincidence. "Nobody expects the Spanish inquisition", I guess. |
${{ '2024/12 These tests use Internet' if 0 }} | ||
or test_decode_gif or test_download_url or "test_get_model[lraspp" |
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.
Really not a fan of this style of commenting...
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.
Me neither. I've filed prefix-dev/rattler-build#1289 for what I believe would be the best way of solving the problem.
- 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 |
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.
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
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.
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.
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.
Let's clean this up with the next PR.
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.
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.
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.
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).
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.
OK, let's get this in, thanks!
- 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 |
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.
Let's clean this up with the next PR.
Checklist
0
(if the version changed)conda-smithy
(Use the phrase@conda-forge-admin, please rerender
in a comment in this PR for automated rerendering)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.