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

Enhance Tensor Flexibility with Structs #327

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

BrianPetkovsek
Copy link

replaces kp::Tensor::TensorDataTypes with std::type_info. This allows you to use any struct as a tensor rather than being limited by base datatypes (int, uint, float, etc..).

The easiest way I found to use structs in shaders is to use the "GL_GOOGLE_include_directive" extension to include a hpp file that holds the struct. this way you can use the struct in your shader code and c++ code. (see new example)

 replaces kp::Tensor::TensorDataTypes with std::type_info. This allows you to use any struct as a datatype

Signed-off-by: Brian Petkovsek <[email protected]>
@axsaucedo
Copy link
Member

@BrianPetkovsek thank you for the contribution, this seems like a reasonable addition - also I wasn't aware of the GL_GOOGLE_include_directive, quite interesting. It seems relatively analogous to the way the Constants are impleemnted as these also have flexibility to be other structures.

In regards to performance would this have any distinction?

As a heads up there seems to still have build issues in the python package - the rest of the examples should also be tested to ensure they work correctly as although a small design change we just need to ensure the rest works - if you can validate the basic examples I can validate the heavier ones like the android / godot.

Signed-off-by: Brian Petkovsek <[email protected]>
Signed-off-by: Brian Petkovsek <[email protected]>
Signed-off-by: Brian Petkovsek <[email protected]>
@BrianPetkovsek
Copy link
Author

BrianPetkovsek commented Jul 7, 2023

Hi @axsaucedo,

I’ve decided to change my approach and use an abstract class called ABCTypeContainer instead of typeid and std::type_info. This allows for the implementation of both a C++ and Python version, which I have already included.

I’ve made some significant changes to main.cpp for the pybind11 bindings, but I’m not entirely sure how push_consts, specconstsvec, and pushconstsvec are implemented. If they’re just buffers, it might be easier to make them a std::vector<byte>.

The C++ type container could use some cleanup. Currently, it uses a counter system with the struct IdCounter. If there’s a way to consolidate this without using an external struct, it should be done.

C++ type container works by
The classId() method in the C++ type container returns a unique identifier for each unique template type instantiation of the TypeContainer template. This is achieved by using a static variable id that is initialized with the value of counter from the IdCounter struct and then incremented, ensuring that each instantiation has a unique id.

The PyTypeContainer works by comparing the dtypes of numpy arrays. A dtype can act as either a structure or just a datatype, allowing for struct-like capabilities (see https://numpy.org/doc/stable/user/basics.rec.html#structured-arrays).

This also resolves #99 as you can put multi-dimensional arrays in structs

Signed-off-by: Brian Petkovsek <[email protected]>
Signed-off-by: Brian Petkovsek <[email protected]>
@BrianPetkovsek
Copy link
Author

Awaiting review/comments.

Copy link
Member

@axsaucedo axsaucedo left a comment

Choose a reason for hiding this comment

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

Thank you for your further iteration @BrianPetkovsek - I have now added a few comments throughout the document.

The main feedback consistent throuhout is to explore how we can 1) keep it as simple as possible - even if we have to reduce functionality 2) ensure determinism, and 3) explicit over implicit.

Going back to my previous question I would be keen to understand if this affects performance - let me know if you can find some at least heuristic based insights.

In regards to your question:

I’ve made some significant changes to main.cpp for the pybind11 bindings, but I’m not entirely sure how push_consts, specconstsvec, and pushconstsvec are implemented. If they’re just buffers, it might be easier to make them a std::vector.

They are indeed just bytes similar to the rest of the data, you can see how these are set in the algorithm and in the opalgo, as well as in the respective Push/SpecConstTest. This means they could benefit from a similar approach as structs can be used. It seems however the current implementation is still failing on the tests - you should be able to run the tests locally with act

As mentioned in the comments however I would be keen to avoid where possible large changes in the C++ API just to make the Python API work, I know this wasn't the major driver but looking at the complexity increase from the first iteration to now I would be keen to explore ways to decrease the complexity of this.

@@ -83,7 +83,11 @@ def build_extension(self, ext):
description='Kompute: Blazing fast, mobile-enabled, asynchronous, and optimized for advanced GPU processing usecases.',
long_description=long_description,
long_description_content_type='text/markdown',
ext_modules=[CMakeExtension('kp')],
ext_modules=[CMakeExtension('kp/kp')],
packages = find_packages(where="python/src"),
Copy link
Member

Choose a reason for hiding this comment

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

Why is this being removed?

Copy link
Author

Choose a reason for hiding this comment

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

just puts the module into a folder (kp), then init imports kp.

was using it for testing if I needed to create a pure python class, I Implemented PyTypeContainer then moved it to c++.
It can be reverted back but really it just allows the ability to package pure python with the project if needed.

setup.py Show resolved Hide resolved
setup.py Show resolved Hide resolved
src/OpTensorCopy.cpp Outdated Show resolved Hide resolved
private:
size_t classId()
{
static size_t id = counter++;
Copy link
Member

Choose a reason for hiding this comment

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

This introduces non-deterministic behaviour, one run TypeContainer will be different to another run - we should avoid. The typeid + std::typeinfo seemed more robust, what's the reason to not use here?

Copy link
Author

Choose a reason for hiding this comment

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

changed to typeid

python/src/main.cpp Outdated Show resolved Hide resolved
}

class PyTypeContainer : public ABCTypeContainer
Copy link
Member

Choose a reason for hiding this comment

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

Reading this it seems that this new iteration of the implementation was added to explore the implementation on the python side, but it seems this is also adding quite a lot of complexity on the C++ side (also there's quite a few python-internal references here that I'm not sure about) - I would be keen to explore how complexity can be reduced in this implementation

{
public:
PyTypeContainer(py::object clazz)
: clazz_(clazz)
Copy link
Member

Choose a reason for hiding this comment

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

I don't follow what's going on here, what's class_(...) in this case?

Copy link
Author

Choose a reason for hiding this comment

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

class_ should be renamed to dtype. It stores the dtype so we can cast the data back to the correct format in the method data() (line 139). Like wise we return the dtype in data_type() (line 161)

Copy link
Author

Choose a reason for hiding this comment

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

renamed for better understanding

auto frombuffer = np.attr("frombuffer");

auto dtype =
dynamic_cast<PyTypeContainer*>(&(*self.dataType()))->clazz_;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I follow - what is class_ in this case (same Q as above)?

Copy link
Author

Choose a reason for hiding this comment

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

clazz_ should be renamed to dtype.

we need the dtype to cast the array from byte to the original dtype

Copy link
Author

Choose a reason for hiding this comment

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

Fixed an issue causing tests to fail, the base object was being overwritten by frombuffer. fixed the implementation.

python/src/main.cpp Outdated Show resolved Hide resolved
@BrianPetkovsek
Copy link
Author

fixed the issues with the tests.

I created a new class called Buffer. This class is a basic buffer class that holds Buffer data like pointer location, size, length, and end. positions. The class gets compiled away at compile time.

I have also updated the classes that uses push/spec constants to allow for Buffer without affecting the external api, (you can still use vectors as push/spec).

@BrianPetkovsek
Copy link
Author

Awaiting review/comments.

Signed-off-by: Brian Petkovsek <[email protected]>
Signed-off-by: Brian Petkovsek <[email protected]>
Signed-off-by: Brian Petkovsek <[email protected]>
@BrianPetkovsek
Copy link
Author

Im compiling my build with MSVC , seems like gcc is more picky with compiling, fixed the build issues.

@BrianPetkovsek
Copy link
Author

I'll downloads gcc and fix it

@axsaucedo
Copy link
Member

Thank you for having a deeper look into this, I was able to get an initial review. I have a few followups:

I have also updated the classes that uses push/spec constants to allow for Buffer without affecting the external api, (you can still use vectors as push/spec).

Would you be able to provide further context on the reasoning for having a Buffer class? Is this to simplify the way that the python wrapper implements it? I feel this buffer adds extra complexity - I do agree that this is only used internally so may not be as bad, but I am wondering if this would be necessary or whether it's only for use in the python side?

On a side note, in regards to naming conventions we should not use conventions that exist in Vulkan to avoid confusion (ie Buffer =? vkBuffer)

In regards to the dataType it seems it's passed everywhere as a pointer, is there a reason why this is not just passed everywhere as reference? GIven its deterministic behaviour I would assume this coudl now be the case.

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.

2 participants