Extending the likelihood factory function loading

[vitenti]

In a recent PR #341 we added the option to load a factory function directly from a module. In this same PR @tilmantroester asked if we could extend this to allow different names for factory functions (instead of always build_likelihood).

We propose the following solution:

likelihood_source = ${FIRECROWN_DIR}/a/a.b.c.  # always fails
likelihood_source = ${FIRECROWN_DIR}/examples/xxx/abc.py # Tries to load abc.py and call build_likelihood from it
likelihood_source = a.b.c.build_likelihood # Tries to load a module called a.b.c and call build_likelihood
likelihood_source = a.b.c # Tries to load a module called a.b.c and call build_likelihood (because it is the default name)

likelihood_source = a.b.d.foo  # Tries to load a module called a.b.c.d and call foo
likelihood_source = a.b.d.bar  # Tries to load a module called a.b.c.d and call bar

We can also have a second configuration variable where one can put the name of the factory function.

[tilmantroester]

Could there be a conflict if foo is both a function in d and a sub-module of d?

[marcpaterno]

Do you mean if someone has:

# in file d.py
import foo    # this adds 'foo' to the global namespace of module 'd'; the value is a module object

def foo():     # this replaces 'foo' in the global namespace of module 'd'; the new value is a function object
    pass

If the import were done after the def for the function, then d.foo would be associated with the module object.
It seems that whichever definition is last is the name that users of the module will see.

I think pylint will complain about the redefinition, so I think we should be able to avoid such cases -- I suspect they would be quite confusing to most people.
I certainly found it confusing -- I checked the above with the documentation and with some testing under Python 3.11.

[tilmantroester]

I was thinking more along the lines of a directory structure

foo/
|-__init__.py
|-bar/
  |-__init__.py

with

# foo/__init__.py
def bar():
    pass

Then

import foo
foo.bar # function foo.bar()

and

import foo.bar
foo.bar # module foo.bar

Which is obviously something to avoid but might be a bit more subtle to catch. It's not caught by pylint either as far as I can tell.

[marcpaterno]

I propose we introduce a new name likelihood_factory_function, to be the string form of the name of the function we are to load from the right module. It is always a function name, so there can be no confusion with a module. If the user does not specify this value, we use a default build_likelihood.

I also propose we introduce the new name likelihood_factory_module to be used the same way as likelihood_source, and that we deprecate (but not yet remove) likelihood_source.

With this, existing configurations remain working as they always have. New configurations can be used with multiple factory functions in the same module, but they must be requested by name.

[marcpaterno]

We have added the ability to have multiple factory functions in the same module with #450, merged into Firecrown v1.8.0.