[proposal] New resolver configuration#938
[proposal] New resolver configuration#938tiran wants to merge 1 commit intopython-wheel-build:mainfrom
Conversation
tiran
force-pushed
the
proposal-new-resolver-config
branch
3 times, most recently
from
ceb5b48 to
071d46c
Compare
|
@shifa-khan has been looking at one of the other tickets related to similar work. |
|
I have inspect our downstream repo. Most
|
tiran
force-pushed
the
proposal-new-resolver-config
branch
from
071d46c to
3356160
Compare
| - `resolver_dist.sdist_server_url` is replaced by `index_url` parameter. | ||
| All `pypi-*` profile support a custom index. | ||
| - `git_options.submodules` is not needed. Like pip, Fromager will always | ||
| clone all submodules. |
There was a problem hiding this comment.
In our downstream, we have packages that explicitly need submodules=False and cases where submodules point to private repos or have broken URLs. These packages when migrated to YAML would deviate from current behavior. Do we have a workaround to address such cases?
For example, adding an optional submodules parameter with true as the default?
There was a problem hiding this comment.
I'm curious, which package fails to build when submodules are enabled? Do you know? I was under the impression that engineers added submodules=False when the package did not need submodules.
Yes, we have a workaround: keep the Python plugin.
If we really need a submodules: false flag for several packages, then we could consider the feature in the future. I would like to start with a simple configuration (80/20 rules).
There was a problem hiding this comment.
Yeah, my bad - let me rephrase. We have packages (tacozip, tpu_info, vllm_neuron) that use submodules=False, which could be for optimization or maybe to avoid actual failures. There's also the future case where submodules might point to private repos or broken URLs. But as you said, keeping a plugin for those edge cases should work.
There was a problem hiding this comment.
I investigated our downstream packages with submodules=False. None of the git repos have git submodules. xla has a .gitmodules file, but it is empty.
|
downstream has few plugins call |
tiran
left a comment
tiran
left a comment
There was a problem hiding this comment.
downstream has few plugins call ensure_pkg_info() in prepare_source as boilerplate. Git clones never have PKG-INFO, but build tools (like setuptools-scm) need it before building. Could the git-based providers (gitlab-git, github-git, pypi-git) handle this automatically?
Great point! Yes, this will be handled automatically in two ways:
- Fromager already adds
PKG-INFOto the final sdist. In some cases, the file is added too late for some PEP 517 hooks. - Fromager will add a
.git_archival.txtfile to clones. Lala is working on the feature in #961 / #962 . This works for all setuptools-scm cases.
I'll update the design with the PKG-INFO and .git_archival.txt details.
| - `resolver_dist.sdist_server_url` is replaced by `index_url` parameter. | ||
| All `pypi-*` profile support a custom index. | ||
| - `git_options.submodules` is not needed. Like pip, Fromager will always | ||
| clone all submodules. |
There was a problem hiding this comment.
I'm curious, which package fails to build when submodules are enabled? Do you know? I was under the impression that engineers added submodules=False when the package did not need submodules.
Yes, we have a workaround: keep the Python plugin.
If we really need a submodules: false flag for several packages, then we could consider the feature in the future. I would like to start with a simple configuration (80/20 rules).
tiran
force-pushed
the
proposal-new-resolver-config
branch
from
3356160 to
6559cd3
Compare
|
@Mergifyio rebase |
Add design proposal for new resolver configuration. See: python-wheel-build#937 Signed-off-by: Christian Heimes <cheimes@redhat.com>
✅ Branch has been successfully rebased |
tiran
force-pushed
the
proposal-new-resolver-config
branch
from
6559cd3 to
37d1bea
Compare
📝 WalkthroughWalkthroughDocumentation for "Fromager Enhancement Proposals" is introduced by creating a new proposals index page and linking it from the main documentation homepage. The first proposal documented specifies a new top-level Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/proposals/new-resolver-config.md`:
- Line 9: Replace the incorrect verb in the sentence "This enhancement document
proposal a new approach to configure the package" by changing "proposal" to
"proposes" so the sentence reads "This enhancement document proposes a new
approach to configure the package"; update the single sentence in the
docs/proposals/new-resolver-config.md content where that exact phrase appears.
- Line 187: Replace the lowercase platform name "github" with the capitalized
"GitHub" in the phrase referring to the `release_artifact` parameter of the
`github` or `gitlab` provider so the documentation consistently uses the
official name; update the occurrence that mentions "github" (in the context of
`release_artifact` and provider descriptions) to "GitHub" while leaving code
identifiers/backticks intact if they must remain lowercase.
- Line 169: The comment contains a duplicated word "must" in the text "tag must
must v1.2+midstream.1.cpu"; edit that comment to remove the duplicate so it
reads "tag must v1.2+midstream.1.cpu" (preserve the rest of the comment and the
Version("1.2+midstream.1") example).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26a9cf54-1ff6-456b-89d4-e20a9f61d2f0
📒 Files selected for processing (3)
docs/index.rstdocs/proposals/index.rstdocs/proposals/new-resolver-config.md
|
|
||
| ## What | ||
|
|
||
| This enhancement document proposal a new approach to configure the package |
There was a problem hiding this comment.
Fix grammar: "proposal" → "proposes".
The verb form is incorrect in this sentence.
📝 Proposed fix
-This enhancement document proposal a new approach to configure the package
+This enhancement document proposes a new approach to configure the package📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| This enhancement document proposal a new approach to configure the package | |
| This enhancement document proposes a new approach to configure the package |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/proposals/new-resolver-config.md` at line 9, Replace the incorrect verb
in the sentence "This enhancement document proposal a new approach to configure
the package" by changing "proposal" to "proposes" so the sentence reads "This
enhancement document proposes a new approach to configure the package"; update
the single sentence in the docs/proposals/new-resolver-config.md content where
that exact phrase appears.
|
|
||
|
|
||
| def matcher_factory_pat(ctx: context.WorkContext) -> re.Pattern | resolver.MatchFunction: | ||
| # tag must must v1.2+midstream.1.cpu and results in Version("1.2+midstream.1") |
There was a problem hiding this comment.
Fix duplicate word in comment.
The comment contains a duplicate word "must must".
📝 Proposed fix
- # tag must must v1.2+midstream.1.cpu and results in Version("1.2+midstream.1")
+ # tag must match v1.2+midstream.1.cpu and results in Version("1.2+midstream.1")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # tag must must v1.2+midstream.1.cpu and results in Version("1.2+midstream.1") | |
| # tag must match v1.2+midstream.1.cpu and results in Version("1.2+midstream.1") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/proposals/new-resolver-config.md` at line 169, The comment contains a
duplicated word "must" in the text "tag must must v1.2+midstream.1.cpu"; edit
that comment to remove the duplicate so it reads "tag must v1.2+midstream.1.cpu"
(preserve the rest of the comment and the Version("1.2+midstream.1") example).
| ### Deprecations | ||
|
|
||
| - `download_source.url` is handled by `pypi-download` profile or | ||
| `release_artifact` parameter of `github` or `gitlab` provider |
There was a problem hiding this comment.
Capitalize "GitHub" for consistency.
The official platform name should be capitalized as "GitHub" to match its usage elsewhere in the document.
📝 Proposed fix
- `release_artifact` parameter of `github` or `gitlab` provider
+ `release_artifact` parameter of `GitHub` or `GitLab` provider📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| `release_artifact` parameter of `github` or `gitlab` provider | |
| `release_artifact` parameter of `GitHub` or `GitLab` provider |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~187-~187: The official name of this software platform is spelled with a capital “H”.
Context: ...le or release_artifact parameter of github or gitlab provider - `download_sourc...
(GITHUB)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/proposals/new-resolver-config.md` at line 187, Replace the lowercase
platform name "github" with the capitalized "GitHub" in the phrase referring to
the `release_artifact` parameter of the `github` or `gitlab` provider so the
documentation consistently uses the official name; update the occurrence that
mentions "github" (in the context of `release_artifact` and provider
descriptions) to "GitHub" while leaving code identifiers/backticks intact if
they must remain lowercase.
3 participants
Add design proposal for new resolver configuration.
See: #937
Summary by CodeRabbit
Release Notes