Autodiscovery of backends, pipelines and validators

[mostafa]

Hey @thomaspatzke and the community,

I added a simple guide for migrating backends to support autodiscovery. Meanwhile I was talking with @kelnage about removing reliance on the backends to have module-level global variables for detection of backends and other relevant functions and classes. My proposal is that we can do this by enumerating objects exported by the module and checking if the object is a class and a subclass of the Backend class. It is much easier to show in the code:

import importlib
import inspect
from sigma.conversion.base import Backend
m = importlib.import_module("sigma.backends.loki")
for obj in m.__dict__:
    if inspect.isclass(getattr(m, obj)) and issubclass(getattr(m, obj), Backend):
        print(f"Yay 🎉 - found {obj}")
# Yay 🎉 - found LogQLBackend

The autodiscovery method is calling importlib.import_module via the _discover_module_directories private method, so why not just extract those classes while doing this via the above code.

Feel free to disregard the suggestion if you've already considered this before.

WDYT?

P.S. I can do it in a PR if you are positive about this.

[thomaspatzke]

I love it! Please do the PR 😉

[mostafa]

@thomaspatzke I've made this PR a while ago. See if this is what you want to go forward with: #119. I listed the reasonings behind my decisions in the PR comments.

[thomaspatzke]

Cool! Thanks for your PR and sorry for let you waiting, was quite busy with various stuff. Some comments:

• Not all packages contain type hints, which hinders the use of type hints for auto-detecting pipeline functions/generators. So I added a warning for those that doesn't have type hints.

I'm thinking about possible solutions:

• Making typing hints mandatory.
• Introduction of a decorator that marks pipelines for autodiscovery like here:

def autodiscover(f):
    f.autodiscover = True
    return f
@autodiscover
def pipeline():
    ...

pipeline.autodiscover is then True. I'm not really sure about the optimal solution because first is somehow implicit and l can imagine that it will confuse some developers. On the other side: behavior can be documented, it's contained in the cookiecutter template and enforces type hints. What do you think?

I think it could be better to remove the warning because there can be functions for valid reasons.

Because it's a breaking change I would introduce it in the 0.10 release, I have possibly another breaking change that could be accumulated there.

[mostafa]

I am in favor of type hints and I always advocate for it. However, the decorator also seems like a good solution. Yet, I can propose two other solutions:

1. Make pipelines as static methods of a class that inherits the Pipeline that we create:
   • The function would remain the same.
   • There is no need to instantiate the class for the static method.
   • You can store other things on the class data type.
2. Make a class decorator that acts as an intermediate transitional state between the current functions and the new Pipeline class, and it will also satisfy your decorator proposal. This is the plan (I prefer):
   • Create a Pipeline class, that can act as a class that can be inherited or used as a decorator:

     class Pipeline:
         def __init__(self, func):
             self.func = func
         def __call__(self, *args, **kwargs):
             return self.func(*args, **kwargs)

   • Use it as decorator or inherited class for the future:

     @Pipeline
     def some_pipeline_func():
         ...

   • The above can be detected as a class by using isinstance function:

     isinstance(some_pipeline_func, Pipeline) // True

   • And since it is a regular Python class, it can also be subclassed:

     class SomePipelineClass(Pipeline):
           ...
     
     issubclass(SomePipelineClass, Pipeline) // True

We can eventually remove the warnings as well.

[thomaspatzke]

Nice Python-magic 😉 My preferred solution would be 2. We then have clearly identifiable classes that still have the same simple semantics.

by the way, did you confused pipelines and validators?

[mostafa]

Python solves problems automagically. 😆 I also recommend the 2nd one and agree with the simple naming conventions.

I was totally confused and hence messed up the names. 🤦 I edited and fixed the comment.

[thomaspatzke]

Have you some time to implement it and provide a PR? I've put it on my todo list, but currently working on another topic.

[mostafa]

I was on vacation. I'll look into it this today or next week.

Update:
Made a PR:

• Experimental pipelines #130

[thomaspatzke]

I "fixed" a test to reflect for the new changes:

* The backend names are now class names, because all the backend classes does not contain a proper `name` property.

I would improve it as follows:

1. If there is a name attribute, this will be used as backend identifier.
2. If there is none, the backend name will be derived from the class name by the following scheme:
   1. A possible Backend at the end of the class name is removed.
   2. The remaining part will be split at capital letters.
   3. Each part will be lower-cased and concatenated with an underscore in between.
      Example: SomeArbitraryBackend will get the identifier some_arbitrary.

What do you think about this?

[mostafa]

LGTM!

[mostafa]

I'll fix this in a separate PR.

[mostafa]

@thomaspatzke I made a PR to handle backend names and this is how it decides:

1. Try to get the obj.name attribute.
2. Try to get the obj.__name__ attribute.
3. Try to get the obj.__class__.__name__ attribute.
4. Convert the name to snake_case while removing the "Backend" suffix.
5. If we still have the "base" backend, return the module name instead.
6. and 7. If we still don't have a name or anything goes wrong, return the default.

• Apply backend naming convention #134

[mostafa]

@thomaspatzke I made a new PR:

• Experimental pipelines #130

[thomaspatzke]

Great! Merged! 😊

[mostafa]

@thomaspatzke Is there anything else I need to do on this to make autodiscovery work more smoothly? Any ideas, discussions, etc.?

[thomaspatzke]

I think for the first it's done and working well. Perhaps there will be some corner cases when this gets into the CLI and needs to be able to discover all existing backends. Do you see any further challenges?

[mostafa]

I am thinking about adding tests to the CLI to ensure things work smoothly.