Variable.get type hints are incorrect

[Samreay]

Bug summary

The Variable.get type hints indicate they return an optional string.

@classmethod
    @sync_compatible
    async def get(cls, name: str, default: Optional[str] = None) -> Optional[str]:
        client, _ = get_or_create_client()
        variable = await client.read_variable_by_name(name)
        return variable if variable else default

However this would be true only if the code was updated to be:

        return variable.value if variable else default

Right now it returns an optional str (if default) or variable (if not)

Version info (prefect version output)

Version:             2.20.1
API version:         0.8.4
Python version:      3.11.8
Git commit:          f6bebfca
Built:               Fri, Aug 9, 2024 10:35 AM
OS/Arch:             linux/x86_64
Profile:             default
Server type:         ephemeral
Server:
  Database:          sqlite
  SQLite version:    3.37.2

Additional context

No response

[Samreay]

Im quite confused here, because I just forked prefect to fix this, and I can see the get method in the fork has an as_object and a better ternary, added three months ago. And yet I swear the prefect version 2.20.1 is newer than that. Will close this out until I can figure out if this is user error or an issue in one of the other variables.py files in the repo.

[Samreay]

Reopening because a reinstall of unpinned prefect via pip install prefect (after doing a removal) shows the incorrect return. If anyone knows why this looks different to my fork, please let me know

[desertaxle]

Thanks for the issue @Samreay! I believe you're correct that the return type doesn't match the type hint. Would you be up for submitting a PR to fix this? I lean towards updating the code to match your suggestion of

return variable.value if variable else default

since I think the type hint matches the original intended behavior for these methods.

[cicdw]

Ah I think I see one part of the confusion - @Samreay the main branch of the repo is being released as the release candidates for 3.0, so it's different from the 2.x releases (which can be found on the 2.x branch). It does appear that there were non-trivial changes to variables.py between the two (such as the introduction of as_object).

[Samreay]

Ah, that makes sense. Yeah I'll submit a PR for this tomorrow then :)

[Samreay]

Okay raising a question here of what the correct behaviour should be. @desertaxle pointed out in the PR that the tests need updating, which means they assume a Variable is returned. This makes sense to me... except for the case when there's a default.

I don't think anyone wants the usage pattern to look like:

var = Variable.get("something", default="default")
var_value = var.value if isinstance(var, Variable) else var

However, if we follow the type hint in that you get a str back, then the tags become inaccessible.

I note that this issue is solved on the main branch due to the as_object kwarg, though I don't think the main branches implementation is great (default is ignored if as_object is true, even if the variable isn't set, which is unintuitive even if documented in the docstring).

So a few options:

1. Update the type hint to be Optional[Variable | str] to reflect current behaviour. Downstream code will have to do isinstance checks.
2. Return the variable value and never the variable. Tags are inaccessible. As theres no way to fetch variables based on tags I dont think anything valuable is lost.
3. A variable is returned always. The default branch returns a (not uploaded temp) variable with the name and value (default) set

Will wait for dev preference before continuing

[desertaxle]

Very well put, @Samreay! I lean towards option two since that will match the current type hint, and I think these Variable methods should be optimized for convenience. If users want to get the tags for a variable, they can use the lower-level PrefectClient to do so.

I would like a second opinion from @cicdw though, since none of the options jump out to me as obviously correct.

[cicdw]

Thanks for tagging me in @desertaxle - my opinion is that this feels confusing because these should be two distinct methods that return different types, not one method that returns two types based on a kwarg. I'm not sure what the right spelling should be, but maybe:

var = Variable.load("something") # mirrors Block loading
assert isinstance(var, Variable)

val = Variable.get("something", default="default")
assert isinstance(val, str)

I think what I'm proposing is a breaking change, so it may need to be exclusive to main / 3.0.

[Samreay]

I think a load vs get difference is the way to go, and this could be updated for the 3.0/main branch with its as_object kwarg coming out.

However, @cicdw do you have an opinion about what to do for the 2.x branch? The only non-breaking change suggested is option one in my suggestion, but I would (as an end user) code for option 2 along with @desertaxle

[cicdw]

I don't think we can introduce a breaking change to 2.x, especially with 3.0 coming out; if it would unblock your use case, I'd accept a new kwarg on get that allows you to specify that you only want the value (name for kwarg TBD):

# default for `value_only` should be `False` for backwards compatibility
val = Variable.get("something", default="default", value_only=True)

and a corresponding update to the type hint on the output to Optional[Union[Variable, str]]. It's not great, but we can safely make the implementation sane in 3.0.

[Samreay]

Honestly if there can't be breaking changes, I'd be tempted to simply correct the type hint and leave it there. Adding a kwarg this late in the 2.x cycle for what is probably a minimally used feature (as this issue hasn't be raised before) seems like adding complexity when the focus should just be on the 3.0 release.

On that note, prior to the 3.0 release, I'd be keen to have a discussion as if the API defined there is what you want. Specifically, the API exposes mutually exclusive kwargs default and as_object.

My query would be:

1. Why both returning the object and not just the value?
2. If there is value in the Variable object, why not just return the default as a Variable?

[cicdw]

Yea that sounds good - if you're open to it, I'll make a PR to 3.0 for this and tag you as a reviewer @Samreay; thanks for working with us on this!

[cicdw]

This was addressed in https://github.com/PrefectHQ/prefect/pull/15070 and in https://github.com/PrefectHQ/prefect/pull/15188 for 3.0.