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

Adding new cmake/scikitbuild core based workflow #85

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

drisspg
Copy link

@drisspg drisspg commented Apr 5, 2024

No description provided.

@drisspg drisspg marked this pull request as draft April 5, 2024 20:08
@ClementPinard
Copy link
Contributor

Is this repo still being updated ? Last pushed commit was 5 years ago, so I though it was mainly archived, and seeing a Meta employee submitting a new PR after all this time is weird.

Anyway, I suppose it's still very early development since I can see all sorts of driss_torch mentions in the code.

The introduction of a Makefile is actually very interesting, but I suppose the tutorial should mention that somewhere as well

@drisspg
Copy link
Author

drisspg commented Apr 9, 2024

@ClementPinard This repo has been archived for some number of time. There has been an influx of new cpp/cuda extensions in the PyTorch Community; flash-attention, mamba, bitsandbytes... One common problem is that extensions dont interact correctly with torch.compile. @zou3519 and I are going to work on updating this repo to provide clean templates for users.

The state of this PR is still very drafty

@drisspg drisspg changed the title Adding new pypyroject.toml based workflow Adding new cmake/scikitbuild core based workflow Apr 10, 2024
*.cubin
*.fatbin
build/**
driss_torch/lib/**
Copy link
Author

Choose a reason for hiding this comment

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

fix

build/**
driss_torch/lib/**
compile_commands.json
benchmarks/data
Copy link
Author

Choose a reason for hiding this comment

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

fix

- ufmt == 2.1.0
- libcst == 1.0.1

# missing host field?
Copy link
Author

Choose a reason for hiding this comment

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

fix

find_package(Torch REQUIRED CONFIG)

# simple_cuda source files
file(GLOB_RECURSE CU_SOURCES csrc/*.cu)
Copy link
Author

Choose a reason for hiding this comment

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

not some conditionals on building cuda sources

)

# cache CUDA_ARCHITECTURES, which seems to be reset by Torch
set(TMP_STORE_CUDA_ARCHITECTURES "${CMAKE_CUDA_ARCHITECTURES}")
Copy link
Author

@drisspg drisspg Apr 11, 2024

Choose a reason for hiding this comment

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

I dont think I need this, however pytorch does deviate from regualre cuda cmake builds

MESSAGE(STATUS "CPP_SOURCES: ${CPP_SOURCES}")

add_library(${SKBUILD_PROJECT_NAME} SHARED
${CU_SOURCES}
Copy link
Author

Choose a reason for hiding this comment

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

same here for conditional

# simple_cuda source files
file(GLOB_RECURSE CU_SOURCES csrc/*.cu)
file(GLOB_RECURSE CPP_SOURCES csrc/*.cpp)
MESSAGE(STATUS "CU_SOURCES: ${CU_SOURCES}")
Copy link
Author

Choose a reason for hiding this comment

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

change the message to debug

lib_path = Path(__file__).parent / "lib" / "libextension_cpp.so"
torch.ops.load_library(str(lib_path.resolve()))
torch.ops.load_library(lib_path)
# torch.ops.import_module("my_extension.abstract_impls")
Copy link
Author

Choose a reason for hiding this comment

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

need to right correct abstract impls


# Specify CMake defines:
[tool.scikit-build.cmake.define]
TORCH_CUDA_ARCH_LIST="9.0"
Copy link
Author

Choose a reason for hiding this comment

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

this should be more generic by default

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

Successfully merging this pull request may close these issues.

3 participants