Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature Request] Support loading kernel soruce from relative and absolute path #6819

Closed
marty1885 opened this issue Mar 27, 2024 · 6 comments
Assignees
Labels

Comments

@marty1885
Copy link
Contributor

marty1885 commented Mar 27, 2024

Is your feature request related to a problem? Please describe.
I've been writing out-of-tree Matalium applications over the weekends. Currently I am forced to create a symlink under TT_METAL_HOME to get CreateKernel to load my own kernels.

For example, I've a kernel under /home/marty/copy.cpp. And have attempt to load it

    KernelHandle kernel = CreateKernel(
        program,
        "/home/marty/copy.cpp",
        core,
        DataMovementConfig{.processor = DataMovementProcessor::RISCV_0, .noc = NOC::RISCV_0_default}
    );

This always fails as Metalium unconditionally prepends TT_METAL_HOME to my specified load path.

                 Always | FATAL    | Failed to generate binaries for copy filesystem error: cannot copy: No such file or directory [/home/marty/Documents/not-my-projects/tt-metal//home/marty/copy.cpp] [/home/marty/Documents/not-my-projects/tt-metal/built/0/kernels/copy/13061086063963452508/brisc/kernel.cpp]
terminate called after throwing an instance of 'std::runtime_error'
  what():  TT_THROW @ tt_metal/impl/program/program.cpp:36: tt::exception

I am forced to create a symlink to workaround it. This is not piratical in real world applications.

ln -s /path/to/my_kernels/ $TT_METAL_HOME/my_kernels

Then load it as

    KernelHandle kernel = CreateKernel(
        program,
        "my_kernels/copy.cpp",
        core,
        DataMovementConfig{.processor = DataMovementProcessor::RISCV_0, .noc = NOC::RISCV_0_default}
    );

Describe the solution you'd like

CreateKernel should attempt to interpret absolute path as absolute and load the kernel without prepending TT_METAL_HOME. And relative paths should be treated as such. Searching under TT_METAL_HOME should be a fallback herbivorous.

Describe alternatives you've considered

Alternative APIs like CreateKernel(program, kernel_src_str, etc..) like OpenCL's clCreateProgramWithSource would be nice too. Although I see most applications ended using this kind of API and would strongly prefer the non-alternative solutin.

Additional context
None.

@marty1885 marty1885 added the feature-request External feature request label Mar 27, 2024
@tt-rkim tt-rkim added metal tt-metal issue P3_Nice_to_have labels Mar 28, 2024
@tt-rkim
Copy link
Collaborator

tt-rkim commented Mar 28, 2024

@jliangTT This sounds like small changes to the metal C++ host API + JIT device kernel compile system. (Maybe @pgkeller ?). I've triaged as P3 and put on runtime team board. Anything else I should do?

@pgkeller
Copy link
Contributor

IMHO, this is fairly high priority - the it works now is sorta stupid for end users. Unfortunately it'll have to wait a bit for resources.

@tt-rkim
Copy link
Collaborator

tt-rkim commented Mar 28, 2024

I fired this up to a P1, but it's runtime team's board, so feel free to do whatever with priority.

@jliangTT jliangTT added feature and removed feature-request External feature request labels Mar 28, 2024
@pgkeller
Copy link
Contributor

I suggest:

  1. any path that doesn't start with "/" be treated as absolute, relative otherwise
  2. this means any existing test that has a path that starts w/ "/" will break w/ this change, those have to be found and fixed
  3. update the docs to mention how to create absolute and relative paths

@tooniz
Copy link
Contributor

tooniz commented Mar 28, 2024

@tt-dma , Paul and I had a quick chat, we should do the following:

  • assume the path is absolute and test if it exists, if so use it
  • relative otherwise

That shouldn't break anything today, and absolute path takes precedence over relative if both exists to avoid surprises if absolute path already exists

@tt-dma
Copy link
Contributor

tt-dma commented Apr 1, 2024

PR #6922 implements support for absolute and relative (to TT_METAL_HOME) paths in CreateKernel calls

@tt-dma tt-dma closed this as completed Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants