Problems with direct _imports.check() call

[PhilipMay]

When the __init__.py imports OMLflowCallback the optuna_transformers.py script is executed.
That executes the _imports.check() call which then throws an exception if transformers or mlflow is not installed.
But that should be avoided.

See here:

HPOflow/hpoflow/optuna_transformers.py

Lines 11 to 17 in d1cce5c

	with try_import() as _imports:
	import mlflow
	import transformers
	# do the check eagerly and not in the constructor
	# because OMLflowCallback inherits from transformers.TrainerCallback
	_imports.check()

The solution would be to put the _imports.check() call into the constructor. But that is not possible because OMLflowCallback inherits from transformers.

The only solution I have is to put OMLflowCallback into an factory function that creates an OMLflowCallback and does the _imports.check() in there.

@twolffpiggott what do you think?

[twolffpiggott]

If we want to expose the OMLflowCallback in the __init__ of hpoflow, then we will need to use the factory function design pattern.

However, I think it would also be reasonable to adopt the convention that all submodules with optional dependencies will not be exposed in the top-level package init.

i.e. if a submodule has optional dependencies, it will need to be imported as from hpoflow.optuna_transformers import OMLflowCallback instead of from hpoflow import OMLflowCallback.

Only modules with no optional dependencies would be imported in the main __init__ and exposed for import as e.g. from hpoflow import SignificanceRepeatedTrainingPruner.

[twolffpiggott]

The inheritance is only functional in the sense of declaring the method signatures of the abstract class.

https://github.com/huggingface/transformers/blob/23ab0b6980e8af5e0b42905d8c09d388245a029d/src/transformers/trainer_callback.py#L158

But other transformers objects (e.g. Trainer) rely on there being at least a "dummy" definition of the non-functional methods of the callback.

We could declare the methods directly in our class, but we would then need to keep many more non-functional methods in sync with changes in the transformers library, which will be a bigger maintenance burden in the long term.

[PhilipMay]

thanks @twolffpiggott that was helpful

fixed in fe31845