Skip to content
This repository was archived by the owner on Apr 23, 2026. It is now read-only.

re-introduce the docling fix; fix DISABLE_GPU_ACCELERATION setting via GITHUB_ENV file (backport #3271) - #3277

Merged
mergify[bot] merged 4 commits into
release-v0.24from
mergify/bp/release-v0.24/pr-3271
Apr 14, 2025
Merged

re-introduce the docling fix; fix DISABLE_GPU_ACCELERATION setting via GITHUB_ENV file (backport #3271)#3277
mergify[bot] merged 4 commits into
release-v0.24from
mergify/bp/release-v0.24/pr-3271

Conversation

@mergify

@mergify mergify Bot commented Apr 10, 2025

Copy link
Copy Markdown
Contributor

This is on the path to fix MacOS tests and the gate. It doesn't enable MacOS tests, yet, because functional tests are still failing.

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the
    conventional commits.
  • Changelog updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Functional tests have been added, if necessary.
  • E2E Workflow tests have been added, if necessary.

This is an automatic backport of pull request #3271 done by [Mergify](https://mergify.com).

booxter added 3 commits April 10, 2025 13:45
This reverts commit ae16698.

Signed-off-by: Ihar Hrachyshka <ihar.hrachyshka@gmail.com>
(cherry picked from commit df926b7)
These are not ignored, as one could expect from a shell code.

Signed-off-by: Ihar Hrachyshka <ihar.hrachyshka@gmail.com>
(cherry picked from commit 10fdc69)
Signed-off-by: Ihar Hrachyshka <ihar.hrachyshka@gmail.com>
(cherry picked from commit 5a0b454)
@mergify mergify Bot added CI/CD Affects CI/CD configuration testing Relates to testing release-branch Pull Request directly to a release branch dependencies Relates to dependencies ci-failure PR has at least one CI failure labels Apr 10, 2025
@mergify mergify Bot added the one-approval PR has one approval from a maintainer label Apr 10, 2025
Signed-off-by: Jaideep Rao <jrao@redhat.com>
@mergify mergify Bot removed the ci-failure PR has at least one CI failure label Apr 10, 2025
@booxter

booxter commented Apr 10, 2025

Copy link
Copy Markdown
Contributor

@courtneypacheco @ktdreyer should we really backport a patch that changes dependencies (here we add/bump docling)? Should we instead cap docling instead? (or contrive a way to work with both versions?)

@ktdreyer

Copy link
Copy Markdown
Contributor

I think we really want users to have the latest docling.

@booxter

booxter commented Apr 11, 2025

Copy link
Copy Markdown
Contributor

@dhellmann are you ok with backporting patches to release branches that include bumps for dependencies like docling? Please advise, I don't see a policy on this matter in dev-docs, but it seems wrong, as per my experience in other projects.

@dhellmann

Copy link
Copy Markdown

@dhellmann are you ok with backporting patches to release branches that include bumps for dependencies like docling? Please advise, I don't see a policy on this matter in dev-docs, but it seems wrong, as per my experience in other projects.

What we usually did in OpenStack was to leave the minimum bound in place unless the newer version of the library had backwards-incompatible changes that required code changes in the consuming code. We did that because it allowed vendors distributing the project downstream some latitude about picking up changes in the z-stream releases. If they wanted some fixes, but not others, they would still get a compatible set of packages. It is safe to leave the lower bounds alone because the installer tools default to selecting the newest available package that match the rules.

@booxter

booxter commented Apr 14, 2025

Copy link
Copy Markdown
Contributor

@dhellmann, sadly, it's the case of backward incompatible changes in docling.

The new docling broke instructlab. So we know that new docling versions don't work - without the patch included here. But: the patch being backported also breaks instructlab if older docling is used (hence the minimal version bump).

In this case, what's preferred: to cap docling and leave instructlab code intact? Or to bump docling, forcing existing users of a release branch to upgrade dependencies?

(A third option would be to make instructlab gracefully handle both versions, but it would require type inspection to detect which docling interface is present; there's no patch to do that. But theoretically, it's possible to write one.)

@dhellmann

Copy link
Copy Markdown

@dhellmann, sadly, it's the case of backward incompatible changes in docling.

The new docling broke instructlab. So we know that new docling versions don't work - without the patch included here. But: the patch being backported also breaks instructlab if older docling is used (hence the minimal version bump).

In this case, what's preferred: to cap docling and leave instructlab code intact? Or to bump docling, forcing existing users of a release branch to upgrade dependencies?

In order to deliver the fix downstream in the product we need the newer docling and the code changes that go with it. So while it's not ideal, it seems like the necessary solution.

(A third option would be to make instructlab gracefully handle both versions, but it would require type inspection to detect which docling interface is present; there's no patch to do that. But theoretically, it's possible to write one.)

That would be another option, but I'm not sure it's worth the effort. I don't know how many people upstream are using the branch, and we know we need the new code downstream.

@mergify
mergify Bot merged commit 0167813 into release-v0.24 Apr 14, 2025
@mergify mergify Bot removed the one-approval PR has one approval from a maintainer label Apr 14, 2025
@mergify
mergify Bot deleted the mergify/bp/release-v0.24/pr-3271 branch April 14, 2025 20:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

CI/CD Affects CI/CD configuration dependencies Relates to dependencies release-branch Pull Request directly to a release branch testing Relates to testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants