Query parsing: numeric flex attributes

jakabadambalazs · March 16, 2020, 10:00am

Do you agree with me that this is unexpected behaviour?:

$ beet ls mood_happy:0.5..1 -f

I get no results and by examining the query I see that the above query was parsed to:

AndQuery([SubstringQuery('mood_happy', '0.5..1', False)])

By hacking into the parser and making it yield this:

AndQuery([NumericQuery('mood_happy', '0.5..1', False)])

Everything works as expected.

I am working on the beets-goingrunning plugin which makes extensive use of these numeric flex attributes. In the plugin I created a custom parser which is based on a hardcoded list of attribute names used to indicate for which attributes should the NumericQuery be used.

I don’t really like the idea of the hardcoded list (I might move it to the plugin’s configuration) but it does the job for now. Do you have any better ideas? Also, could this be something to be integrated into the core parser?

adrian · March 16, 2020, 12:53pm

You’ll want to mark your field mood_happy with an integer type:
https://beets.readthedocs.io/en/stable/dev/plugins.html#flexible-field-types

Then the query parsing will work as you expect it to.

jakabadambalazs · March 16, 2020, 1:37pm

WooooW! You guys have thought of everything !

jakabadambalazs · March 16, 2020, 8:15pm

It works like a charm (I just cut out 200 lines of code).

jakabadambalazs · March 18, 2020, 11:19pm

but it makes my plugin incompatible with acousticbrainz…

Traceback (most recent call last):
  File "/Users/jackisback/opt/miniconda3/bin/beet", line 8, in <module>
    sys.exit(main())
  File "/Users/jackisback/opt/miniconda3/lib/python3.7/site-packages/beets/ui/__init__.py", line 1266, in main
    _raw_main(args)
  File "/Users/jackisback/opt/miniconda3/lib/python3.7/site-packages/beets/ui/__init__.py", line 1249, in _raw_main
    subcommands, plugins, lib = _setup(options, lib)
  File "/Users/jackisback/opt/miniconda3/lib/python3.7/site-packages/beets/ui/__init__.py", line 1148, in _setup
    library.Item._types.update(plugins.types(library.Item))
  File "/Users/jackisback/opt/miniconda3/lib/python3.7/site-packages/beets/plugins.py", line 344, in types
    u'another type.'.format(plugin.name, field)
beets.plugins.PluginConflictException: Plugin goingrunning defines flexible field danceable which has already been defined with another type.

Do you have any ideas how it could be solved whilst keeping both plugins active?

jakabadambalazs · March 18, 2020, 11:37pm

Actually, it looks like a bug.

The goingrunning plugin and the acousctibrainz plugin define the danceable attribute with the exact same type: 'danceable': types.Float(6).

Looking at the check here:

github.com

adamjakab/Beets/blob/00c6d1439ee29b3b4592d6f65cfa57b1d1f96575/beets/plugins.py#L342


      
              return out
          
          
          def types(model_cls):
              # Gives us `item_types` and `album_types`
              attr_name = '{0}_types'.format(model_cls.__name__.lower())
              types = {}
              for plugin in find_plugins():
                  plugin_types = getattr(plugin, attr_name, {})
                  for field in plugin_types:
                      if field in types and plugin_types[field] != types[field]:
                          raise PluginConflictException(
                              u'Plugin {0} defines flexible field {1} '
                              u'which has already been defined with '
                              u'another type.'.format(plugin.name, field)
                          )
                  types.update(plugin_types)
              return types
          
          
          def named_queries(model_cls):

the plugin_types[field] != types[field] is comparing the two instances and not their types. Of course they are different. In fact, this:

if field in types and type(plugin_types[field]) != type(types[field]):

works.

What do you think? Should I open an Issue for this?

adrian · March 19, 2020, 1:14am

Hi! Yes, please do open an issue for this! It would be great to fix it.

However, the right fix is unfortunately not to add type(...) calls. We want to compare the objects, which represent beets data types—not the types of those types, which are Python classes. (I know it’s confusing.) The problem is that the type objects need to define __eq__ or __cmp__ in order to be comparable.

jakabadambalazs · March 19, 2020, 8:58am

Ok, let’s do it! I’ll create the issue and have a go at the fix. I think I understand what you mean by comparing not the built-in types but the classes those instances created from. I never used __eq__ or __cmp__ but there is always a first time.

I am also thinking about proposing a different way beet handles this issue. At the moment it exits with a PluginConflictException. What if we load the plugins in the order they are indicated in the configuration file and we adopt a “first plugin wins” strategy with only a warning when fields are redeclared with a different type? What do you think?

have a great day

jakabadambalazs · March 19, 2020, 1:59pm

My plugin suffers of this issue only in one specific point. That is when I parse the query string composed from the command line queries + those picked up from the configuration file for a specific selection. If I do not override the types (the way you showed me) the parsed query will be wrong (for my needs). If I do the application will crash because of the bug mentioned above. So, this would be a no-go situation.

However, I really want to create a new release of my plugin and I want it to work. And I also want it to play nicely along with the acousticbrainz plugin and potentially any other plugin that might declare these attributes.

So, this is what I came up with:

    def _retrieve_library_items(self, training: Subview):
        full_query = self._gather_query_elements(training)

        # Store a copy of defined types and update them with our own overrides
        orig_types = library.Item._types.copy()
        override_types = common.get_item_attribute_type_overrides()
        library.Item._types.update(override_types)

        # Execute the query parsing (this will use our own overrides)
        parsed_query = parse_query_string(" ".join(full_query), Item)[0]

        # restore the original types
        library.Item._types = orig_types.copy()

        self._say("Song selection query: {}".format(parsed_query), log_only=True)
        return self.lib.items(parsed_query)

I don’t think it needs much explanation. It’s a hack but it works great. It works with AB plugin activated and the query gets parsed correctly.

Obviously I do not have the insight into Beets as you do.
Do you see any issues with this solution?

adrian · March 19, 2020, 2:13pm

Seems pretty good for now! Of course, the real fix will be in beets itself (or in just disabling one or the other plugin for now), but this should do in the interim.

adrian · March 19, 2020, 2:19pm

I should also have mentioned: it should work to let one plugin specify the type. That is, if you have the acousticbrainz plugin enabled and it declares the types for all the fields you care about, that should be enough—it shouldn’t be necessary to also declare the same type in your plugin. (Of course, that would prevent people who only want to use one plugin from taking advantage of the types.)

adrian · March 19, 2020, 2:21pm

Oops, and I missed this one above:

To be honest, I think we should not do this. It would make things more confusing for end users—they would be able to see that one plugin declares a field with type A, and another plugin makes it have type B, and it would be unclear why one plugin “succeeded” in this declaration and the other “failed.” I think it’s better for the plugins to just resolve their conflict and avoid declaring differing types, hence the ugly exception.

jakabadambalazs · March 19, 2020, 2:30pm

Sure, that totally makes sense. But my plugin can not depend on what the AB plugin declares or on how it declares them. Also, many people will not have the AB plugin enabled so I do need to have my own implementation that works.

At one point I also thought about checking if the AB plugin was loaded and only when it was not I declared my fields. It also worked. However, the ‘hack’ solution should be compatible also with other plugins not only AB.

jakabadambalazs · March 19, 2020, 2:32pm

OK. It was a wild idea

k03n · May 1, 2020, 7:35pm

I ran into the same issue as described by Adam above, but not while writing a plugin.
Is there a way to declare the attribute type from the cli directly?

For reference: I have added an index field to sort and/or select series of albums out of a larger collection.

adrian · May 1, 2020, 9:03pm

Yes! Use the types plugin:
https://beets.readthedocs.io/en/stable/plugins/types.html

k03n · May 1, 2020, 9:32pm

Great, thank you for your swift response!

Topic		Replies	Views
Can't use those AcousticID Tags? (Queries involving non-integer ranges) Help	2	983	May 18, 2017
How to create a flexible attribute of numeric type Help	2	331	May 10, 2022
Force beets list to list ALL known fields for a query Help	2	542	February 19, 2021
Flexible attributes Help	7	602	November 4, 2020
Queries without accents Development	3	497	March 17, 2021

Query parsing: numeric flex attributes

Related topics