Studio: make the mmproj fallback import resolvable under node's type stripping by danielhanchen · Pull Request #9181 · unslothai/unsloth · GitHub
Skip to content

Studio: make the mmproj fallback import resolvable under node's type stripping - #9181

Closed
danielhanchen wants to merge 1 commit into
mainfrom
fix-mmproj-fallback-node-resolution
Closed

Studio: make the mmproj fallback import resolvable under node's type stripping#9181
danielhanchen wants to merge 1 commit into
mainfrom
fix-mmproj-fallback-node-resolution

Conversation

@danielhanchen

Copy link
Copy Markdown
Member

npm test is red on main right now. tests/queued-model-capabilities.test.ts does not fail an assertion, it dies on module resolution, so the whole file's 4 tests never run.

Error [ERR_MODULE_NOT_FOUND]: Cannot find module
  '.../src/features/chat/utils/mmproj-fallback'
  imported from .../src/features/chat/utils/image-input-support.ts

The file it cannot find is there. The problem is the extension: image-input-support.ts imports ./mmproj-fallback with none, and Node's --experimental-strip-types does not do extension resolution, so it looks for a file called exactly mmproj-fallback and gives up.

That import has been extensionless for a while without anyone noticing, because nothing reached it from a Node test. It became reachable when getImageInputUnavailableReason was added to tests/queued-model-capabilities.test.ts's imports in #9173, which is why it surfaces only now.

Reproduced on a clean checkout of main at 077593612 with nothing else applied:

  • before: npm test reports ✖ tests/queued-model-capabilities.test.ts, and running that file alone gives the ERR_MODULE_NOT_FOUND above.
  • after: the same file reports tests 4, pass 4, fail 0.

The fix is the extension. There are already 46 imports under src/ carrying an explicit .ts for exactly this reason, so this follows what is there rather than introducing a new convention. The alternative, making the test import the module dynamically the way tests/delete-chat-files-preference.test.ts does, would work too but changes a test to accommodate a source file rather than the other way round.

No new test is needed here: tests/queued-model-capabilities.test.ts is itself the guard. It is red without this change and green with it, which is how I verified the fix rather than assuming it.

Found while resolving #9058 against main. It is not that PR's, so it is not being fixed inside it.

…stripping

tests/queued-model-capabilities.test.ts imports image-input-support.ts
directly, and that file imports ./mmproj-fallback without an extension. Node's
--experimental-strip-types does not do extension resolution, so the whole test
file dies with ERR_MODULE_NOT_FOUND rather than failing an assertion.

The import only became reachable from a node test when getImageInputUnavailableReason
was added to that test's imports, which is why it surfaced now. 46 imports under
src/ already carry an explicit .ts for the same reason.
@danielhanchen

Copy link
Copy Markdown
Member Author

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.

1 participant