Add R feedstock v0 -> v1 recipe migration: GenericV0ToV1Migrator + RV0ToV1Migrator - #6381
Add R feedstock v0 -> v1 recipe migration: GenericV0ToV1Migrator + RV0ToV1Migrator#6381pb01ka wants to merge 11 commits into
GenericV0ToV1Migrator + RV0ToV1Migrator#6381Conversation
b1b5443 to
ed2f9c5
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6381 +/- ##
==========================================
+ Coverage 81.27% 81.71% +0.43%
==========================================
Files 146 150 +4
Lines 17537 18013 +476
==========================================
+ Hits 14253 14719 +466
- Misses 3284 3294 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| return | ||
|
|
||
| v1_content, blocking = self._convert(meta_yaml_path.read_text()) | ||
| if blocking: |
There was a problem hiding this comment.
Don't we need to flag this somewhere as an error in the bot or something? This may look like a successful migration otherwise 🤔
There was a problem hiding this comment.
I don't think so. It's because the v1_content is never written into recipe.yaml for this case. The warning being logged is Skipping v0 -> v1 conversion... and the control returns. So migration would never actualise.
May be I am missing something? Do you want to elevate this to an error instead of a warning? Please let me know.
There was a problem hiding this comment.
Who's going to see the warning? I'm expecting something we can track in its status page (e.g. like https://conda-forge.org/status/migration/?name=supportwindowsarm64platform), so unless it's an error, it will be a skipped feedstock (or even worse, a "successfully migrated" one in the eyes of the bot metadata?). Need to verify that.
There was a problem hiding this comment.
I see. It makes sense. Here are a couple (exactly two 😆 TBH) of options I would like to propose,
A. Promote it to a real top-level Migrator. i.e., give it a name, build it as a GraphMigrator (AFAICT, it's currently not) with its own effective_graph/pr_limit, and register it in make_migrators.py like ArchRebuild/OSXArm/MigrationYaml. I think this would be the only option matching the exact status-page precedent you shared.
B. Write a per-node status field via a new MiniMigrator-scoped mechanism. AFAIK, pr_info.pre_pr_migrator_status only covers top-level Migrator exceptions. RV0ToV1Migrator.migrate() would write to attrs["pr_info"], and status_report.py would need extending to read and report it.
I would recommend option A - I guess it is what we intend to do anyways (?). Please let me know TY.
There was a problem hiding this comment.
Ah, true, why is this a minimigrator? 🤔 I had missed that part at the beginning assuming we wanted a full graph migrator.
There was a problem hiding this comment.
I think a mini-migrator makes sense. We can piggy back off r 4.6 migrator
There was a problem hiding this comment.
If the v0->v1 conversion fails, then the PR for r_base 4.6 will still be made right? Then at the end of the migration, we can retry for the handful left.
There was a problem hiding this comment.
Thanks @isuruf I think your comment is quite helpful in converging on a better plan than the standalone-migrator approach I'd have gone with earlier. Here's where I've landed:
1. Mini-migrator, piggy-backed on the r-base 4.6 migration
Reverting RV0ToV1Migrator from a standalone GraphMigrator approach back to a MiniMigrator approach, and scoping it to the r-base 4.6 rebuild migration specifically. I guess this follows the exact precedent already in this codebase for RUCRTCleanup:
if migration_name.startswith("r-base44"):
piggy_back_migrations.append(RUCRTCleanup())The equivalent hook might look like if migration_name.startswith("r-base46"): piggy_back_migrations.append(RV0ToV1Migrator()). Since bumping the r-base pin forces a rebuild of essentially every R feedstock, this gets us close to full coverage of the R feedstock population in one campaign. Importantly, it also combines the r-base 4.6 rebuild and the recipe format conversion into a single PR per feedstock, instead of two separate ones.
One concern worth flagging explicitly: AFAICT, the r-base 4.6 migration doesn't exist yet. I checked that there's no reference to it anywhere in this code base.
Assuming that the r-base 4.6 migration will be created in the near future, and since the make_migrators.py hook is a one-line addition, the RV0ToV1Migrator mini-migrator can be built and merged independently of that, and then piggy-backed onto the r-base 4.6 migration once it exists.
If it already exists, then please point me to it and I'll add the piggy-back hook immediately.
a v0 -> v1 failure shouldn't block the r-base 4.6 PR. I think, as long as RV0ToV1Migrator.migrate() doesn't raise on a skip (it doesn't, it just logs a warning and returns gracefully), the containing r-base 4.6 migrator's PR gets created normally regardless of whether the conversion succeeded, failed, or was skipped for that feedstock.
2. Track failures so the handful left can actually be retried
The catch with (1) alone: AFAIK, a mini-migrator's migrate() return value is discarded by the caller (and also warnings are non-persistent), so no record of which feedstocks were skipped is kept. To retry for the skipped/failed recipe conversions, at the end of the migration wave, we need to have such a record.
So I am thinking of adding a lightweight tracking mechanism (as described in Option B above): when RV0ToV1Migrator.migrate() skips a feedstock, write a small status note into that node's persisted pr_info (e.g. skipped: True + the blocking reason(s)) instead of only logging. It should be just enough data to query "which R feedstocks are still on schema v0, and why" after the migration wave finishes, and drive a manual or scripted retry pass over just those.
Please correct me wherever I'm wrong. TY.
There was a problem hiding this comment.
Ok Isuru's approach seems sensible, let's do that. I don't think we need to track which ones were skipped. Instead, the 2nd sweep will traverse the full R graph and skip any feedstock that is already recipe.yaml.
There was a problem hiding this comment.
I see.
I don't think we need to track which ones were skipped. Instead, the 2nd sweep will traverse the full R graph and skip any feedstock that is already
recipe.yaml.
It would be slightly wasteful to revisit the recipes (going through all the feedstocks and checking if recipe.yaml already exist for each of those) which are already successfully converted. However, I don't mind doing so.
Final decision - I will keep the RV0ToV1Migrator as a mini-migrator for now. I wouldn't go for step 2 (Track failures so the handful left can actually be retried) in #6381 (comment). It's anyways an independent of the logical implementation of the migrator. So, if the need arises in the future, we can add tracking mechanism later on.
With a0e437c, However, with 08ab378, conversion happened. Here are the relevant files, Original Click here{% set version = "1.0.4" %}
{% set posix = 'm2-' if win else '' %}
package:
name: r-kedd
version: {{ version|replace("-", "_") }}
source:
fn: kedd_{{ version }}.tar.gz
url:
- https://cran.r-project.org/src/contrib/kedd_{{ version }}.tar.gz
- https://cran.r-project.org/src/contrib/Archive/kedd/kedd_{{ version }}.tar.gz
sha256: bb944389a61cff22bb3bdde938a0ff6c16700a1ca725ef36b391b97812c45b26
build:
noarch: generic
number: 2
rpaths:
- lib/R/lib/
- lib/
requirements:
build:
- r-base
run:
- r-base
test:
commands:
- $R -e "library('kedd')" # [not win]
- "\"%R%\" -e \"library('kedd')\"" # [win]
about:
home: https://CRAN.R-project.org/package=kedd
license: GPL (>= 2)
summary: Smoothing techniques and computing bandwidth selectors of the nth derivative of a probability density for one-dimensional data.
license_family: GPL3
license_file: '{{ environ["PREFIX"] }}/lib/R/share/licenses/GPL-3' # [unix]
license_file: '{{ environ["PREFIX"] }}\R\share\licenses\GPL-3' # [win]
extra:
recipe-maintainers:
- conda-forge/rConverted Click hereschema_version: 1
context:
version: "1.0.4"
posix: "'m2-' if win else ''"
package:
name: r-kedd
version: ${{ version|replace("-", "_") }}
source:
url:
- https://cran.r-project.org/src/contrib/kedd_${{ version }}.tar.gz
- https://cran.r-project.org/src/contrib/Archive/kedd/kedd_${{ version }}.tar.gz
sha256: bb944389a61cff22bb3bdde938a0ff6c16700a1ca725ef36b391b97812c45b26
file_name: kedd_${{ version }}.tar.gz
build:
number: 2
noarch: generic
dynamic_linking:
rpaths:
- lib/R/lib/
- lib/
requirements:
build:
- r-base
run:
- r-base
tests:
- script:
- if: not win
then: $R -e "library('kedd')"
- if: win
then: "\"%R%\" -e \"library('kedd')\""
about:
license: GPL-2.0-or-later
summary: Smoothing techniques and computing bandwidth selectors of the nth derivative of a probability density for one-dimensional data.
license_file: ${{ (env.get("PREFIX") ~ '/lib/R/share/licenses/GPL-3') if unix else (env.get("PREFIX") ~ '\\R\\share\\licenses\\GPL-3') if win else '' }}
homepage: https://CRAN.R-project.org/package=kedd
extra:
recipe-maintainers:
- conda-forge/r |
|
The steps remaining for this PR to be marked as "Ready for review",
|
…version Wraps conda-recipe-manager's RecipeParserConvert to convert v0 recipes to v1, skipping (with a warning) any conversion crm can't raise/warn its way past unattended. IGNORED_WARNINGS starts with license_family, which crm always flags as a warning even though it already strips the field cleanly.
Add RV0ToV1Migrator, an R-specific subclass
add structural validation of crm's v1 output
crm's message table can report nothing at all for genuinely malformed output (mismatched/nested ${{ }}, leftover duplicate keys), so verify the generated recipe.yaml structurally rather than trusting it blindly.
Refactor _normalize_legacy_license to take a to_spdx callable, and give GenericV0ToV1Migrator/RV0ToV1Migrator their own _to_spdx methods so RV0ToV1Migrator's CRAN-specific "LicenseRef-Unlimited" -> "Unlimited" mapping stays confined to the R subclass instead of the shared migrators/license.py table.
|
I have a couple thoughts on this having worked quite a bit on the last two migrations. First is handling of patches - although Second is handling of build scripts - we'll definitely have to regenerate build scripts (or better yet, move the build steps in to the recipe itself). Existing build scripts reference At the same time we still have some packages with old build scripts which build the package like normal for all platforms except So the question is how much of this the migration bot should try to clean up itself versus flagging the issue for manual intervention? |
|
TYSM @danielnachun for highlighting these important points.
I will complete testing my migrator on a larger sample of R-feedstocks. Subsequently I will use it to figure out answers to your questions (the two points you have highlighted and how much the bot will be able to do on its own) by tomorrow EOD. I hope that will work. TY again. <3. |
|
That's great. If your larger test doesn't already hit some of these snags, I can definitely at least find some recipes with more complicated One other thing I realized now too that can come up - in every R migration we always run into at least a handful of CRAN package which have been archived and are therefore unmaintained (see https://cran.r-project.org/web/packages/RandomFields/index.html for example). Some fraction of those packages will fail to build, and we end up archiving those feedstocks. Potentially it would be worth checking the package page to see if it's archived and instead of migrating it, have the bot open an issue indicating the feedstock should be archived. |
|
Two questions: recipes with a While testing 1. Ambiguous/custom identifiers with no deterministic SPDX equivalent AFAICT, A few ways we could handle this, roughly in increasing order of acceptability:
Would wait for your thoughts on this. 2. Unselector-ed duplicate top-level sections (e.g. Separately,
The recipe is still skipped either way - this only replaces the internal Other than these two questions, from my end I am marking this PR as ready for review. Now I will work on answering @danielnachun's questions. |
|
https://github.com/conda-forge/conda-forge-bot/actions/runs/31217780965/job/92995304151?pr=6381 seems to be unrelated failure. A simple re-run should make it pass I guess. |
@danielnachun I found out the following sample for the two points you shared. I will experiment using these sample and get back to you with a concrete well-tested response. If you have any other feedstocks worth experimenting with, please share with me, I will include it in my sample. |
reason - the install_name_tool fix is dead code, not just possibly-stale
|
@danielnachun As promised, here are my thoughts on your comment above. 1. Patches I think this can stay out of scope for the migrator (please let me know if you think otherwise). I also checked how often this actually comes up and AFAICT, only 1 of our sample of ~350 feedstocks -
2. Build scripts Same as patches, I think we can extend this going forward: catalog and add detectors for concrete, recurring patterns as we find them - same as Please let me know your opinions on both of these. Please also note my comment above. TY. |
|
For 1, I think it's fine not to try to fix patches - at the very least Claude Code is very good at regenerating the patches so it's not a huge maintenance burden. For 2, the pattern of checking the platform in scripts like this should be removed entirely - the I think what I should do is just suffer through cloning the conda-forge/feedstocks repo and then having Claude on my machine locally comb through all the build.sh scripts for the R feedstocks to figure out which feedstocks even have this issue and what kind of patterns are there. The cleanest ideal outcome for this whole thing is that are able to move the vast majority of recipes to have Other thing that was missed is that I thought |
Description
This is step 2 of the R feedstock v0 -> v1 migration plan, split into two layers, both planned in this PR:
GenericV0ToV1Migrator(implemented): aMiniMigratorclass that only knows theconda-recipe-manager(crm) conversion mechanics. It has no R-specific logic.RV0ToV1Migrator(in progress, to be pushed in next commit of this PR): an R-specific subclass layered on top of the base class, adding the R-specific pre/post-processing and registered for R feedstocks. See TODOs below for what's still outstanding.Why split into a base class + subclass
Keeping the
crm-mechanics layer free of R-specifics means:crm's actual behavior (raised exceptions vs. warnings vs. errors), without needing R recipe fixtures.RV0ToV1Migratorcan extendIGNORED_WARNINGSand add its own pre/post-processing without touching the safe-to-auto-convert decision itself.TODOs - remaining work in this PR
RV0ToV1Migrator: layer R-specific pre/post-processing on top ofGenericV0ToV1Migrator:{{ compiler('c') }}/{{ stdlib('c') }}/native/posix/m2w64token cleanup (comparable to whatRUCRTCleanupalready does for the ucrt migration), and multi-output handling for CRAN "recommended" vs. non-recommended packages.RV0ToV1Migratorexists, to catch any remaining edge cases.r-baseitself) currently crash crm'srender_to_v1_recipe_format()with a rawAttributeErrorin its dependency-upgrade logic;RV0ToV1Migratorcatches this and skips the recipe rather than converting it. Add a local workaround so multi-output CRAN "recommended" vs. non-recommended recipes actually convert instead of always being skipped.Cross-refs, links to issues, etc:
https://gist.github.com/pb01ka/4d915b2196470ca7c95643c31212b52b