Optional type inference

[jmosbacher]

Addendum to #569, makes it optional so that we dont immediately break code that relies on the loophole created by issue 568. when we stop getting the warning in straxen tests we can enable the type inference by default.

[JoranAngevaare]

Thanks @jmosbacher ! I was a bit too quick to merge.

• The current PR doesn't seem to have any effect right?
• Could we set limited amount of warnings (without bloating someone's notebook with warnings) because a lot of things will be affected?

[jmosbacher]

@JoranAngevaare

• The current PR doesn't seem to have any effect right?
  The only effect it should have (assuming I didn't make any mistakes) should be raising a warning in cases where the infer_dtype is OMITTED
• Could we set limited amount of warnings (without bloating someone's notebook with warnings) because a lot of things will be affected?
  I actually made a mistake and added the warning in all cases where the infer_dtype is not specified. I changed it to only cases where the type will inferred in the future.

I used a regular warning, but do you think maybe a deprecation is more suitable? I'm on the fence.

[JoranAngevaare]

Suggested change

	if infer_dtype is OMITTED and type is OMITTED and default is not OMITTED:
	warnings.warn(f'You are setting a default value for config {name} but not \
	specifying a type. In the future the type will be inferred from \
	the default value which will result in an error if this config \
	is set to a different type.')
	if infer_dtype and type is OMITTED and default is not OMITTED:
	if infer_dtype is OMITTED and type is OMITTED and default is not OMITTED:
	warnings.warn(f'You are setting a default value for config {name} but not \
	specifying a type. In the future the type will be inferred from \
	the default value which will result in an error if this config \
	is set to a different type.')
	elif infer_dtype and type is OMITTED and default is not OMITTED:

[jmosbacher]

if i set infer_dtype=False wouldnt this still infer the type?

[JoranAngevaare]

fair but if infer_dtype is True because it is a string, so we would get both a warning and later an error in the validate part.

[JoranAngevaare]

just edited the suggestion

[jmosbacher]

oops i pushed an edit before i noticed you also did...

[jmosbacher]

either is fine with me.

[JoranAngevaare]

yeah I think this many if statements are indeed a bit unnecessarily tars to follow but your method is good 👍

[JoranAngevaare]

But the logic didn't do anything right? You want to warn that this will fail?

Anyway, what I meant was that we could use a filter to prevent throwing the same warning 1000 times (but I'm not sure if we would, let's see)

[jmosbacher]

I think a more fitting place to filter the warnings would be in straxen no?

[JoranAngevaare]

Thanks, this helps indeed.

Maybe we should allow type to be a list of types or does a Ty.Union also work?

[jmosbacher]

I think setting the type parameter to a tuple of types should work as is but I guess we could make that explicit in the type hint