add typing in optuna_transformers

[PhilipMay]

@twolffpiggott can you please tell me the type of this?

HPOflow/hpoflow/optuna_transformers.py

Line 47 in e2b0943

def setup(self, args: TrainingArguments, state: TrainerState, model):

[PhilipMay]

@twolffpiggott what do you think? Can we remove this check since we use typing? Or change the check to check for Number?

HPOflow/hpoflow/optuna_transformers.py

Line 126 in e2b0943

if isinstance(v, (int, float)): # TODO: remove or change to Number?

[PhilipMay]

@twolffpiggott I closed the merge request and opened the inline questions here again.

[twolffpiggott]

@twolffpiggott can you please tell me the type of this?

HPOflow/hpoflow/optuna_transformers.py

Line 47 in e2b0943

def setup(self, args: TrainingArguments, state: TrainerState, model):

Am I right to interpret your meaning as for the untyped model argument?

I left it untyped because I was not sure if there is a single Transformers base model type. Looking at this Transformers design document, it might be reasonable to put model: Union[transformers.PreTrainedModel, transformers.TFPreTrainedModel]

[twolffpiggott]

@twolffpiggott what do you think? Can we remove this check since we use typing? Or change the check to check for Number?

HPOflow/hpoflow/optuna_transformers.py

Line 126 in e2b0943

if isinstance(v, (int, float)): # TODO: remove or change to Number?

The type hints aren't restrictive and will not prevent people from using the method incorrectly. We could use something like mypy though I'm not sure if this will cover all dynamic use cases.

Custom types like Numeric can be used for hints but not isinstance checks. Might be simplest to keep as is but here are some alternatives

[PhilipMay]

Am I right to interpret your meaning as for the untyped model argument?

Oops yes!

[PhilipMay]

done