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

feat: Parsers Dynamic Configuration #160

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

dominik737
Copy link
Contributor

@dominik737 dominik737 commented Jan 19, 2025

Implementation of parser dynamic configuration settings and retrieval. Written with intention that the code will be in future ported to C++, hence heavily restricting "Python magic".

The core part of the feature is the Parameter class. The class basically contains getter and setter function reference for a configurable parameter e.g. getIgnoredIndexes&setIgnoredIndexes. Parser can then expose whichever configurable parameters by simply with self._parameters.add.
The Parameter class itself is abstract (cannot be instantiated). To add parameters use it's child classes like BoolParameter or IntListParameter.
In Python-only context the child classes would be redundant and everything they do could be done with runtime reflection, type conversions etc., but for C++ they might be necessary.

Retrieving parser configuration is done by calling configuration property on a parser. The configuration parameter name, value as string, type as string and description will be returned.
Setting the configuration is done with configuration_input, which is standard dai.Node.Input. The input accepts ConfigurationChange message, inherited from dai.Buffer.

@dominik737 dominik737 added enhancement New feature or request parsers Changes affecting ml.parsers labels Jan 19, 2025
@dominik737 dominik737 requested a review from moratom January 19, 2025 20:28
@github-actions github-actions bot added the messages Changes affecting ml.messages label Jan 19, 2025
@dominik737 dominik737 requested a review from klemen1999 January 19, 2025 20:29
@codecov-commenter
Copy link

codecov-commenter commented Jan 19, 2025

Codecov Report

Attention: Patch coverage is 56.94444% with 62 lines in your changes missing coverage. Please review.

Project coverage is 36.64%. Comparing base (7529260) to head (9bc69de).

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...epthai_nodes/ml/parsers/configuration/parameter.py 51.51% 16 Missing ⚠️
depthai_nodes/ml/parsers/base_parser.py 42.10% 11 Missing ⚠️
...des/ml/parsers/configuration/int_list_parameter.py 53.84% 6 Missing ⚠️
...i_nodes/ml/parsers/configuration/bool_parameter.py 58.33% 5 Missing ⚠️
..._nodes/ml/parsers/configuration/float_parameter.py 58.33% 5 Missing ⚠️
...ai_nodes/ml/parsers/configuration/int_parameter.py 58.33% 5 Missing ⚠️
...des/ml/parsers/configuration/runtime_parameters.py 58.33% 5 Missing ⚠️
...epthai_nodes/ml/parsers/classification_sequence.py 50.00% 4 Missing ⚠️
depthai_nodes/ml/messages/configuration_change.py 50.00% 3 Missing ⚠️
depthai_nodes/ml/parsers/classification.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##           main     #160       +/-   ##
=========================================
+ Coverage      0   36.64%   +36.64%     
=========================================
  Files         0       79       +79     
  Lines         0     4153     +4153     
=========================================
+ Hits          0     1522     +1522     
- Misses        0     2631     +2631     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@jkbmrz jkbmrz left a comment

Choose a reason for hiding this comment

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

I'm not really in the loop with C++ porting but this LGTM.

Copy link
Collaborator

@klemen1999 klemen1999 left a comment

Choose a reason for hiding this comment

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

Generally LGTM but I'd wait for @moratom to confirm the structure is good for porting

@@ -73,6 +74,19 @@ def __init__(
self.ignored_indexes = ignored_indexes if ignored_indexes is not None else []
self.remove_duplicates = remove_duplicates
self.concatenate_classes = concatenate_classes
self._configurable_parameters.add(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This + getters for every setter have to be added also to all other parsers right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly. I've marked the PR as draft yet, just to confirm the core logic. If approved I'll add the getters and parameters to all the other parsers.

The Parameter class constructor also takes in optional name and description arguments. Those are for now set dynamically. The name with setter.__name__ and description with setter.__doc__. This is the most "Python magic" used there. And it may not be okay for porting. Resulting in necessity to also have the name and description arguments filled in the Parameter constructor.

I would not expose all the parameters. Namely those that are set by the NNArchive and their change would break the parser for the model. Examples of such methods are: setOutputLayerName, setClasses, setSoftmax.

Copy link

@moratom moratom left a comment

Choose a reason for hiding this comment

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

Nice, should be portable to C++.

I do have one question though - how come we went with a completely generic message instead of a strongly typed, per parser message here?

Any downsides for the strongly typed message?

@dominik737
Copy link
Contributor Author

dominik737 commented Jan 22, 2025

Nice, should be portable to C++.

I do have one question though - how come we went with a completely generic message instead of a strongly typed, per parser message here?

Any downsides for the strongly typed message?

@moratom
Not sure if I understand, but the parsers can be configured with strongly typed methods already. Those strongly typed methods for setting/getting the configuration like getRemoveDuplicates& setRemoveDuplicates are still available.

The reason that the message (ConfigurationChange) is generic, is because in the Viewer we want to have the parsers configurable by user. The user has to know which properties of the parser are configurable, their values and also have a way to configure them "from string" or at least from json.

If you know a way how to make it more strongly typed, while still having a way to configure the parameters by user, let me know and I'll adjust the code.

@moratom
Copy link

moratom commented Jan 23, 2025

The reason that the message (ConfigurationChange) is generic, is because in the Viewer we want to have the parsers configurable by user. The user has to know which properties of the parser are configurable, their values and also have a way to configure them "from string" or at least from json.

If you know a way how to make it more strongly typed, while still having a way to configure the parameters by user, let me know and I'll adjust the code.

@dominik737
What's blocking us of having the configuration message be strongly typed and per parser (so not one for all)?
It would still allow dynamic reconfiguration.
I mean the viewer would have to know this messages upfront, but outside of the viewer strongly typed messages are usually preferred by developers.

(I might be missing something here of-course)

@dominik737
Copy link
Contributor Author

What's blocking us of having the configuration message be strongly typed and per parser (so not one for all)?
It would still allow dynamic reconfiguration.
I mean the viewer would have to know this messages upfront, but outside of the viewer strongly typed messages are usually preferred by developers.

@moratom
I also prefer strongly typed objects. I think most users will use just the ParsingNeuralNetwork node though. The node selects the parser for the user at runtime based on the provided NNArchive. Thus the user has no clue what the underlying parser type is.

On the other-side, when user knows the underlying parser type. He can just retrieve the parser from the ParsingNeuralNetwork and use the strongly typed methods that are there e.g. getRemoveDuplicates& setRemoveDuplicates. I acknowledge that this is not the way DAI does things (configuration messages), but it is the way parsers are designed right now (not by me).

In the Viewer I could technically have a big switch with all the parsers and their capabilities. But to me it felt as a worse option than having this information provided by the parser itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request messages Changes affecting ml.messages parsers Changes affecting ml.parsers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants