global: accept pure dict RECORDS_REST_FACETS config variable

[zazasa]

Look at the example for such config variable https://github.com/inveniosoftware/invenio-records-rest/blob/master/examples/app.py#L174 .
The filters and post_filters fields are functions and the https://github.com/inveniosoftware/invenio-records-rest/blob/master/invenio_records_rest/facets.py expects a function to execute.
In order to use systems as Rancher where you can create a different config for each instance, we should be able to provide a pure dict config variable.
So I propose to accept a dictionary (for example {'terms':{'field':'fieldname'}} ) in the filters and post_filters fields.

[jirikuncar]

So I propose to accept a dictionary

@zazasa instead of a dictionary, it should accept an importable string pointing to a callable.

[lnielsen]

Is the main problem here only that you can't use Rancher?

The are functions because they take URL parameters as input:

/search?type=article&type=preprint

filter_func(['article', 'preprint']) -> {'terms': {'types': ['article', 'preprint']}}

[zazasa]

ok. I understand the problem, so the importable string seems the best solution, but we need to provide even the argument to pass to the function (the field for the term filter).

[lnielsen]

#myfilters.py
article_filter = term_filter('mycustomfield')

#config.py
.... 
'filters': {'mycustom': 'myfilters.article_filter'}

[zazasa]

@lnielsen : it makes no sense to have a config variable like that because you hardcode the filter field instead of configure it in the variable. 'mycustomfield' should be configurable in the config.py .

[lnielsen]

Is what you propose something along the lines of?

#config.py
.... 
'filters': {'myfacet': ('invenio_records_rest.facets.term_filter', 'myfield')}

IMHO above just wraps a function call and adds complexity the facets code. The output of terms_filter('myfield') is a callable with takes a single argument values. I'm fine with adding support for either providing 1) a callable or 2) an import string to a callable. However extending to also providing arbitrary arguments to the callable I don't think is a good idea.

As I understand it, the main argument is that you want a pure dictionary in the configuration. However, many other places in Invenio also does not support pure dictionary configuration, hence if we decide to go with pure dictionaries in Records-REST we should apply it consistently throughout Invenio.

I think what you are trying to do can also be achieved by some simple scripting the configuration:

# config.py
def setup_filters(env_options):
   # ....
   return {k: terms_filter(v) for k, v in options}

# ...
'filters': setup_filters(os.env['MY_RANCH_FILTERS']),

Alternatively, there's also the possibility to customise the config loading via http://pythonhosted.org/invenio-config/usage.html. See dirty example at https://github.com/zenodo/zenodo/blob/master/zenodo/factory.py#L65-L73

[audub]

This has nothing to do with Rancher or any other external software. It is the result of using environment variables which in the end will be interpreted as a string because of the literal_eval function (which fails when you give it functions):
https://github.com/inveniosoftware/invenio-config/blob/a9c4411cd1beb84e1430031c2af30b84e2fe25c5/invenio_config/env.py#L58
which again means you cannot configure the system using environment variables.

We have a way to do it for now, but I think this is a valid request to find a nice standard for configuring all parts using environment variables?

[lnielsen]

@audub As mentioned, I'm ok with providing an import string callable so something like in first example works. I disagree with something like my second example.

Regarding environment variables I think the work very well for simply things like SQLALCHEMY_DATABASE_URI. I don't think the work well for dumping a large Python dictionaries like RECORDS_REST_FACETS or RECORDS_REST_ENDPOINTS. For large Python dictionaries I'd rather see the instance file config used instead which is also easily implementable in Docker:

FROM myinveniobase
COPY custom.cfg /opt/invenio/var/instance/custom.cfg

[audub]

Your code implies building a new docker image for each instance. Even though it is based on a base, that will quickly become chaos for us I think. Running it with pure configs is much quicker in our deployment setup, and way easier to test. Everyone shares the same image, and the customisations from there are configs. That does not mean that we can not put instance specific files in the image and use environment variables to choose which ones to use, but it is not ideal.

And facets is something that is set up differently for each instance, so it would be nice to find a way to configure them easily. We already have something close to your scripting example, so we will stick with that for now.

For me we can close this, and if we find another better way of doing it, we can revisit later.

[lnielsen]

And facets is something that is set up differently for each instance, so it would be nice to find a way to configure them easily.

Don't get me wrong here. I'm happy to find better ways to configure the facets. I have objections to one approach, but doesn't mean that we cannot find another way, and I'm happy to brain storm possible solutions with you.

We already have something close to your scripting example, so we will stick with that for now.

Perhaps your function could be made part of Records-REST? Then env var configuration would be something like:?

RECORDS_REST_FACETS = setup_env_facets()

Alternatively we can try to find a way to deal with ENV var configuration for large dictionaries in a generic way in Invenio-Config.

Or perhaps you have other ideas for how to do it nicely?

[lnielsen]

@audub @zazasa On monday we're starting a 2-weeks Invenio sprint. I'm not sure yet if we will get around to Records-REST, however if we do, we need to take a decision on this issue. I suggest we meeting during next week and take a decision.