feat(project_override): add add_dynamic_field to project_override#1105
feat(project_override): add add_dynamic_field to project_override#1105
add_dynamic_field to project_override#1105Conversation
Allows declaratively adding PEP 621 dynamic fields to pyproject.toml via package settings YAML, replacing the need for custom prepare_source plugins for this common operation. - New `add_dynamic_field` list in `project_override` settings - Validates entries against PEP 621 allowed field names - Merges with existing `[project] dynamic` without duplicates Closes: #934 Co-Authored-By: Claude Code <noreply@anthropic.com> Signed-off-by: Josef Skladanka <jskladan@redhat.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds support for appending PEP 621 Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_pyproject.py (1)
182-198: ⚡ Quick winAssert the build-system stays untouched in the dynamic-field-only path.
This test only checks the new
[project]table, so it would still pass ifPyprojectFixrewrote[build-system].requiresand injectedsetuptoolsbehind the scenes. Please add an assertion for the existing build-system contents so this regression is caught.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_pyproject.py` around lines 182 - 198, The test should also assert that the existing [build-system] table is unchanged after running PyprojectFix; parse the original PYPROJECT_TOML to get the expected build-system and compare it to the resulting document. In test_pyproject_add_dynamic_field_no_project_section, after writing PYPROJECT_TOML and before/after fixer.run(), compute expected_build_system = tomlkit.loads(PYPROJECT_TOML)["build-system"] (or parse it once from the constant) and add an assertion like assert doc["build-system"] == expected_build_system to ensure no unintended modifications to build-system.requires or other keys by PyprojectFix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fromager/pyproject.py`:
- Around line 169-182: The current branch calls PyprojectFix(...).run() whenever
add_dynamic_field is set, which causes unnecessary [build-system] normalization;
change the logic so dynamic-only updates don't trigger the build-system rewrite:
if add_dynamic_field is true but both update_build_requires and
remove_build_requires are false, either (A) handle the dynamic-field update in a
separate path (e.g., call a new helper that only edits [project].dynamic) or (B)
extend PyprojectFix (class PyprojectFix and its run method) to accept a flag
like skip_build_system_normalize and pass that flag from the caller (the block
using pbi.build_dir(...) and PyprojectFix(...).run()) so when only
add_dynamic_field is requested PyprojectFix does not normalize or insert
setuptools into [build-system].
- Around line 141-158: In _update_dynamic_fields ensure you normalize the
existing project["dynamic"] before comparing: deduplicate and sort old_dynamic
(e.g., normalized_old = sorted(set(old_dynamic))) and also ensure merged is
deduplicated and sorted, then compare normalized_old to merged; if they differ
assign project["dynamic"] = merged and log the change referencing
old_dynamic/merged (use normalized_old in the comparison/log so
reordering/duplicates are fixed even when no new fields are added).
---
Nitpick comments:
In `@tests/test_pyproject.py`:
- Around line 182-198: The test should also assert that the existing
[build-system] table is unchanged after running PyprojectFix; parse the original
PYPROJECT_TOML to get the expected build-system and compare it to the resulting
document. In test_pyproject_add_dynamic_field_no_project_section, after writing
PYPROJECT_TOML and before/after fixer.run(), compute expected_build_system =
tomlkit.loads(PYPROJECT_TOML)["build-system"] (or parse it once from the
constant) and add an assertion like assert doc["build-system"] ==
expected_build_system to ensure no unintended modifications to
build-system.requires or other keys by PyprojectFix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1774ea49-574b-4d05-9fbc-22351aba2f80
📒 Files selected for processing (7)
docs/customization.mdsrc/fromager/packagesettings/__init__.pysrc/fromager/packagesettings/_models.pysrc/fromager/pyproject.pytests/test_packagesettings.pytests/test_pyproject.pytests/testdata/context/overrides/settings/test_pkg.yaml
| def _update_dynamic_fields(self, doc: tomlkit.TOMLDocument) -> None: | ||
| """Merge new entries into ``[project] dynamic``.""" | ||
| if not self.add_dynamic: | ||
| return | ||
| project: TomlDict = doc.setdefault("project", {}) | ||
| old_dynamic: list[str] = project.get("dynamic", []) | ||
| merged = list(old_dynamic) | ||
| for field in self.add_dynamic: | ||
| if field not in merged: | ||
| merged.append(field) | ||
| merged.sort() | ||
| if set(merged) != set(old_dynamic): | ||
| project["dynamic"] = merged | ||
| logger.info( | ||
| "changed project dynamic from %r to %r", | ||
| old_dynamic, | ||
| merged, | ||
| ) |
There was a problem hiding this comment.
Normalize before comparing.
The current set(...) check hides changes that only remove duplicates or reorder the list, so pre-existing malformed [project].dynamic arrays never get cleaned up unless a new field is added.
♻️ Suggested fix
- old_dynamic: list[str] = project.get("dynamic", [])
- merged = list(old_dynamic)
- for field in self.add_dynamic:
- if field not in merged:
- merged.append(field)
- merged.sort()
- if set(merged) != set(old_dynamic):
+ old_dynamic: list[str] = list(project.get("dynamic", []))
+ merged = sorted(set(old_dynamic) | set(self.add_dynamic))
+ if merged != old_dynamic:
project["dynamic"] = merged🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fromager/pyproject.py` around lines 141 - 158, In _update_dynamic_fields
ensure you normalize the existing project["dynamic"] before comparing:
deduplicate and sort old_dynamic (e.g., normalized_old =
sorted(set(old_dynamic))) and also ensure merged is deduplicated and sorted,
then compare normalized_old to merged; if they differ assign project["dynamic"]
= merged and log the change referencing old_dynamic/merged (use normalized_old
in the comparison/log so reordering/duplicates are fixed even when no new fields
are added).
There was a problem hiding this comment.
I don't agree.
- The set() comparison is intentional. You don't want to trigger a write (and a log message saying you "changed" something) just because the existing list wasn't sorted. The function should only touch the
file when it actually adds a new field. - The suggested fix introduces a behavioral change: it would now rewrite dynamic even when add_dynamic contributes nothing new, purely to normalize ordering/duplicates
There was a problem hiding this comment.
@jskladan, you're right — I'll withdraw the suggestion. The set() comparison correctly answers "did we actually add a new field?", since merged is always a superset of old_dynamic. My proposed merged != old_dynamic would have incorrectly triggered rewrites for pre-existing ordering or duplicate differences, which is outside the scope of this function. The current logic is sound.
|
The implementation here looks basically good, but the design raises UX questions. Do we need to edit any "static" fields? Or other fields in the pyproject.toml? Will we have separate fields in the settings file for each of those things? That seems like it's going to be cumbersome for a user, since they will have to use separate inputs in our settings to modify different settings in the pyproject data. How could we express the modifications more generally? |
That is probably a question for @Lanceypantsy but personaly, I think the That said, I'd like to stress once again, that I was coming at this based on what the #934 contains, and If the scope should be broader (e.g. a generic pyproject patching mechanism), I think that's a separate design discussion and a separate ticket may be worth it. I'd rather not overengineer this MR beyond what was asked for, as I certainly do not have the required in-depth understanding and expertise WRT Fromager. |
…ly updates Guard build-system path in PyprojectFix.run() to prevent setuptools injection when only add_dynamic_field is set. Addresses CodeRabbit review feedback. Co-Authored-By: Claude Code <noreply@anthropic.com>
|
That's fair, although it depends on how many of these we accumulate over time. If we have 10 of them for modifying different parts of the pyproject data structure and they have no coherent unified design, it's going to be hard for a new user to figure out how to use them.
That's more or less what I was thinking of as a more general solution, yeah.
I can understand that. At the same time, I want to solve the underlying problem, not just the one described in the ticket. That's why I was asking questions about other types of fields we might need to modify, to get a sense of how broad the problem is. |
| "remove_build_requires": ["cmake"], | ||
| "update_build_requires": ["setuptools>=68.0.0", "torch"], | ||
| "requires_external": ["openssl-libs"], | ||
| "add_dynamic_field": ["readme"], |
There was a problem hiding this comment.
I do not think we have any existing asserts testing this. I will suggest something like this to suitable tests assert dict(doc["project"].items())["dynamic"] == ["readme"]
|
I crosschecked downstream and this will replace 1 existing plugin (pydeck.py) . cc @tiran |
We don't need that plugin any more. Lance's upstream PR visgl/deck.gl#10048 was accepted and merged. The fix is in the latest releases of pydeck. |
|
So, given this will replace no actual plugins, should we just put it to ice, and have a discussion about possibly changing the In the mean time, I made AI do some analysis (for whatever it is worth, I did not verify the numbers):
tldr (complete thing attached here ):
...
And TBH, I don't believe even the proposed |
|
In downstream, we deliberately do not support structured patching of installation requirements. Our builds should have the same dependencies with the same version constraints as upstream. We only support modifications of build requirements to address custom build systems and breaking changes in setuptools. For example, some upstream projects use a custom build script or build environment instead of using Python's build system requirements. It took us a while to convince projects to declare a build dependency on Torch, too. The fact is also covered by https://fromager.readthedocs.io/en/latest/proposals/new-patcher-config.html
Installation requirements need to be patched in a different way. Patching pyproject.toml dependencies only works for static dependencies. Build backends can provide dynamic installation dependencies, too. A portable and generic solution has to patch the |
|
Yeah, so what I'm reading here is, that the original requirement from #934 is no longer one, and we don't have a use-case for any other changes to the pyproject.toml on top of what we already support. So... Let's close both the ticket and the pr as obsolete/wontfix or something? |
|
I'm coming in late to this conversation, and only have a small amount of context to add. When I wrote the pydeck plugin originally, @EmilienM asked me to file the issue to be able to do this programatically with a setting instead of plugin. If analysis shows that this is not a common pattern that we need to support to minimize plugins, I concur that the issue and PR should be closed. |
|
OK, feels like the concsensus is to close this. We can discuss a more holistic approach in a new ticket, if necessary. |
5 participants
Allows declaratively adding PEP 621 dynamic fields to pyproject.toml via package settings YAML, replacing the need for custom prepare_source plugins for this common operation.
add_dynamic_fieldlist inproject_overridesettings[project] dynamicwithout duplicatesCloses: #934
[X] PR follows CONTRIBUTING.md guidelines