Fix Invalid: Refactor operations and tensorfactory

[willayy]

Description

I have added the possibility to change the implementation behind our tensor operations multiple times. You do, however, need to set it for all the types you want. Unfortunately, this is necessary as templates only exist at compile time.

I went for a solution with a static class TensorOperations (a class with only static members) with a static function for add, subtract, multiply ,gemm, sliding window etc... Each method also got its own setter and storage pointer. I went for this approach for these following reasons:

• Classes allows us to get visibility modifers so we can hide the storage pointers from tampering which isn't included in namespaces, using classes also follows our object oriented design approach.
• Using a simple setter, getter approach we don't have to modify any of our existing code. Its also very simple to work with and does not require a lot of extra hours of trial and error to implement. We could have used something more sophisticated here but since we are running out of time and this solution is acceptable imo.
• I moved the avx and intel functions to new files to signal that these are extensions of the default implementations.
• I made type aliases for the function types to improve readability.
• I decided to keep all functions in a single class since it makes the operations easier to use with no apparent downside.

One thing that is kind of annoying is that I have to make a setter that's the same for all functions which adds a bit of annoying overhead work. If somebody has a good solution to this please tell me.

Issue

Closes #210

[ehmc123]

This is not quite as I had pictured it. One of my biggest concerns with our codebase now is maintainability, as the project grows it is becoming more unreadable and then also harder to debug.

So the point I was trying to make about modularity on Monday was basically dividing the project by concern. And I believe our code for modularity i.e. factories/function pointers should be completely separate from implementations like our main code that is not hardware specific or hardware specific like avx512 and so on.

So basically what I wanted to see to ensure the maintainability of the projects was separation like this:

/modularity layer ( Whatever name we want )
-- Here we store the architecture/code for the the functions pointers and factories and so. Basically a "layer in between" that isn't concerned about actual implementation of the pointers it just stores the pointers.

/implementations
--/default (not hardware specific can run on all platforms )
-- Here we have like all nodes, tensor, gemm, arithmetic implementations.

--/avx512
-- This maybe only need to change gemm and arithmetic so it only has that.

--/cuda
-- And another hardware specific implementation example

-- All implementations should have an easy install.hpp or something similar, and basically it would quick install all pointers to the modularity layer.

/abstract classes (both modularity layer and implementation will be reliant on abstract classes)

This way we would clearly separate our codebase by concerns increasing readability and improving maintainability, all these folders could even be submodules.

If you have a different idea please comment it, but I believe this a pretty crucial time to think about maintainability.

[willayy]

@ehmc123 I will provide a reply this afternoon!

[willayy]

@ehmc123

The problem (not a problem I guess) I see with implementing an install function is that for it to be useful you kind of need to make implementations for all functions available in TensorOperations which is often not the case. For example, avx only concerns the gemm functions so If you want to use it you can just use the gemm setter. The only reason I kept this code

  // Pointer to the gemm std::function. Different defaults depend on the compiler
  // flags
#if defined(USE_AVX_GEMM)
  template <TensorConcept::Types T>
  static inline toft::gemm_func<T> gemm_ptr = mml_gemm_avx<T>;
#elif defined(USE_AVX512_GEMM)
  template <TensorConcept::Types T>
  static inline toft::gemm_func<T> gemm_ptr = mml_gemm_avx512<T>;
#elif defined(USE_BLOCKED_GEMM)
  template <TensorConcept::Types T>
  static inline toft::gemm_func<T> gemm_ptr = mml_gemm_blocked<T>;
#else
  template <TensorConcept::Types T>
  static inline toft::gemm_func<T> gemm_ptr = mml_gemm_inner_product<T>;
#endif

Is for legacy reasons. But if you want I think it's a reasonable compromise to remove this part and use the setters where it's necessary to do so. Making it more readable.

But to finish my point about install I don't think its a bad idea necessarily, we could actually use it as a handy shortcommand that just uses the set_pointers but I just think its not that important at this moment.

Also, I'm still not sure what you mean by factories, the TensorFactory is an implementation of the Factory pattern for making tensors but TensorOperations is by definition, not a Factory, I would call it a utility class.

I don't think there is a viable case for making TensorOperations have an abstract base class since it is static and doesn't interact with other parts of the code in a polymorphic way. If you want you could always subclass it and extend but since we already made it possible for the user to change implementations via the setters this is pretty much only for if you want to add new functions to it.

One of my biggest concerns with our codebase now is maintainability, as the project grows it is becoming more unreadable and then also harder to debug.

I agree with you on this one and there are many reasons why that's an issue! But I don't TensorOperations and TensorFactory is a part of the problem here. TensorOperations/TensorFactory does not depend on any implementations at all, yes we set the MML functions as defaults but that's just for the sake of convenience, if we didn't you would always have to start a test or program with set_xyz_ptr... or Install:default... which I just think becomes annoying and many other libraries uses defaults in this manner.

Im also thinking you might mean that the file tree of our project is a bit confusing (which I agree with). I have created a branch called feature/repo-restructuring where I aim to make it more readable. Im very open to collaborate on this if you are interested and have the time!

I think its really important we are united and on the same page for this so I'm very open to continue this discussion! If you want we could also talk on discord I'm available the whole day today!

[ehmc123]

@willayy

Yes mostly what I am asking about is the structure of our file tree, that is a big part of assuring maintainability! But I don't really understand the reasoning of treating hardware specific implementations (like AVX) of Gemm separately when it comes to modularity.

If we truly want runtime modularity I don't see why we should use build time flags to control which implementations is used. Most use cases of our modularity will be probably be hardware specific Gemm or Arithmetic, so I think we should just treat them as separate implementations to keep within the design. I think it is very confusing mixing runtime modularity with build flags.

What I am calling factories is merely a blanket term for the same thing, there are of course better words for this which is why I called it the modularity layer in the comment above, both tensorfactories and tensoroperations although they are using different patterns fill the same purpose of being an abstract layer between implementations which we can choose which why I am lumping them together.

[willayy]

@willayy

Yes mostly what I am asking about is the structure of our file tree, that is a big part of assuring maintainability! But I don't really understand the reasoning of treating hardware specific implementations (like AVX) of Gemm separately when it comes to modularity.

If we truly want runtime modularity I don't see why we should use build time flags to control which implementations is used. Most use cases of our modularity will be probably be hardware specific Gemm or Arithmetic, so I think we should just treat them as separate implementations to keep within the design. I think it is very confusing mixing runtime modularity with build flags.

What I am calling factories is merely a blanket term for the same thing, there are of course better words for this which is why I called it the modularity layer in the comment above, both tensorfactories and tensoroperations although they are using different patterns fill the same purpose of being an abstract layer between implementations which we can choose which why I am lumping them together.

@ehmc123 Sure I can understand that, it wasn't my idea to set the the avx instructions using macros. I think its just a solution that somebody implemented "just for the moment, we can change it later". I will remove it!