Dependency and Build Fixes #171
Conversation
facebook-github-bot
added
the
CLA Signed
neuralcompression/ext/__init__.py
Outdated
|
|
||
| from torch.utils.cpp_extension import load | ||
|
|
||
| print(str(Path(__file__).resolve().parent / "pmf_to_quantized_cdf_py.cc")) |
There was a problem hiding this comment.
Nit: factorize of Path(__file__).resolve().parent into extension_folder and reuse below. If you plan to load more extensions like this, you might create a function load_extension :
def load_extension(extension_name: str, file_names: List[str]):
extension_folder = Path(__file__).resolve().parent
return load(
name=extension_name,
sources=[str(Path(__file__).resolve().parent / file_name) for file_name in file_names],
verbose=True,
)
There was a problem hiding this comment.
Okay let me try it :).
There was a problem hiding this comment.
@QuentinDuval I updated this to try to follow your suggested format. Does it fit what you were thinking?
There was a problem hiding this comment.
@QuentinDuval I am more than happy about @mmuckley's change but I am also open to other ideas for managing extensions.
There was a problem hiding this comment.
I think the main question I would be curious to have an answer for is "on what platform does the CppExtension work" and what to do to make it work on all of them. I know that torchvision does have a lot of extensions, but for instance, I never tried to install it on windows.
There was a problem hiding this comment.
My current solution follows this thread. Based on this I think it will fail on Windows. The nice thing is with this new implementation we localize it to only this function.
The better thing later would be to figure out a multi-platform distribution framework and move this back to setup.py or to remove the C++ code.
There was a problem hiding this comment.
@mmuckley Frankly, in retrospect, it was overkill on my behalf. But, in my defense, I didn't have a strong grasp of what did and did not work with @torch.jit.script at the time (e.g., a Python implementation should be easily compilable).
There was a problem hiding this comment.
@0x00b1 I'm open to any implementation in any format! My only requirement is that if we have C++ code we pay the cost of distributing it.
|
@QuentinDuval @0x00b1 any actionable requests on this PR? Otherwise I need one of you to approve so we can merge. It's not a perfect fix but I think it allows distribution. |
LGTM, I approved it :) |
|
@mmuckley LGTM. I apologize for the delay. I wanted to read up on |
|
No worries @0x00b1! |
This is a potpourri of fixes.
Dependencies
Currently, we require very specific package versions for any install. This is too restrictive for a research package where researchers may have a variety of reasons to carefully tailor their environment. This PR resolves the issue by allowing general installation to have flexible packages. To maintain CI stability and functionality, the fixed version requirements have been moved to
dev.Build
We build our CPP extension using
loadinstead ofsetup.py. This removes PyTorch as a dependency at install time and removes the need for us to distribute binaries. This required addingninjaas a dependency. Hopefully we can get rid of it eventually, but this temporarily should fix distribution.CI testing
isortwas misconfigured and missed a lot of import changes. I fixed the config and ran it on all files to update import sorting.Other
While implementing this I also found a few other minor issues with respect to versioning, tests, and formatting that I resolved.
This PR makes NeuralCompression require Python 3.8 due to its usage of
importlib.Testing
Testing done via CI.