Skip to content

qa: fix ty caching and align CI with local run#46278

Merged
tarekziade merged 5 commits into
mainfrom
tarek-ty-tightening
May 29, 2026
Merged

qa: fix ty caching and align CI with local run#46278
tarekziade merged 5 commits into
mainfrom
tarek-ty-tightening

Conversation

@tarekziade
Copy link
Copy Markdown
Collaborator

@tarekziade tarekziade commented May 29, 2026

What does this PR do?

I noticed make typing was failing locally but the ty checker was green in CI. This happened because the CI run did not error on warnings.

There was also a bug in how we cache calls: when Ty was failing via make typing, the next cached call was appearing green because it was not busting the cache when ty errored out.

This patch fixes those discrepancies

@tarekziade tarekziade requested review from SunMarc and remi-or May 29, 2026 06:45
@HuggingFaceDocBuilderDev
Copy link
Copy Markdown

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Comment thread src/transformers/generation/continuous_batching/distributed.py Outdated
Cherry of danyalahmed1995:fix/46170-device-mesh-import-guard, applied as a
base so this branch can be rebased onto main once #46205 lands (it will then
drop out as a duplicate).
@tarekziade tarekziade force-pushed the tarek-ty-tightening branch from c2e896f to b6bb7f9 Compare May 29, 2026 07:35
@tarekziade
Copy link
Copy Markdown
Collaborator Author

tarekziade commented May 29, 2026

[DONE] now picks the change from #46205 so the changes on src/transformers/generation/continuous_batching/distributed.py will dissapear once we rebase with the landed PR

@tarekziade tarekziade requested a review from ydshieh May 29, 2026 07:37
Comment thread utils/check_types.py
# previously clean `types` run can be reused from cache.
# Approximate: ty follows imports beyond the listed directories; these globs cover
# the explicitly targeted paths but not transitive dependencies.
# ty follows imports *beyond* the checked roots, so the cache key must cover every source file
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but the scope is always within src/transformers, right? i.e. it will never go beyond this?

Copy link
Copy Markdown
Collaborator Author

@tarekziade tarekziade May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but we check a subset of the files in that directory with our set up.
the change makes sure that we bust the cache in case ANY file has changed because it can indirectly impact the ty call.

Comment thread utils/check_types.py
# `make typing` and CI both pass and the issue is never caught before commit.
result = subprocess.run(
["ty", "check", "--respect-ignore-files", "--exclude", "**/*_pb*", *directories],
["ty", "check", "--respect-ignore-files", "--error-on-warning", "--exclude", "**/*_pb*", *directories],
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Locally, we have this --error-on-warning but not on current CI ??
Or I misunderstand the inconsistency you described?

I noticed make typing was failing locally but the ty checker was green in CI. This happened because the CI run did not error on warnings.

Copy link
Copy Markdown
Collaborator Author

@tarekziade tarekziade May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was a combination:

  • no error on warnings
  • the local can have more packages installed : torch, torchvision, etc

the combo made the two environments behave differently when warnings where triggered.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i will leave @remi-or for this file

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's done in the other PR that got merged. I have just rebased so its gone from this one

@tarekziade tarekziade added this pull request to the merge queue May 29, 2026
Merged via the queue into main with commit fda83a3 May 29, 2026
38 checks passed
@tarekziade tarekziade deleted the tarek-ty-tightening branch May 29, 2026 08:02
Copy link
Copy Markdown
Member

@SunMarc SunMarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants