Deprecate Compute function data classes

[rjmello]

Marked the ComputeFunctionDocument and ComputeFunctionMetadata classes for deprecation. This change reflects an early design adjustment to better align with the existing Globus Compute SDK.

---

📚 Documentation preview 📚: https://globus-sdk-python--1092.org.readthedocs.build/en/1092/

[sirosen]

Excellent!

It's maybe not surprising since we've already discussed this, but I have nothing but positive things to say about this changeset.

EDIT: Because it now appears contradictory, this is an exemplary implementation of the change as discussed. But now, per the comment thread below, we may be revisiting the antecedent decision-making. I stand by having nothing but praise for the implementation. 😄

[kurtmckee]

It looks like wholly removing function_data is a breaking change, not a deprecation, and so is adding data without a default value.

I think that function_data probably needs to stay in place, and be made mutually exclusive with data.

[sirosen]

I advocated this position in the initial discussion, but did not feel strongly enough that I wanted to block people's work over it.
My preference would be that we simply leave the argument name as function_data since there's very little reason to change it, but I'll leave room for others to make their case.

Perhaps we can go back to the situation that I would like best.

[rjmello]

@sirosen, @derek-globus and I discussed this earlier today and concluded that the breaking change is worth doing because the minimal ComputeClient is brand new, minimally functional, and not yet utilized by the Compute SDK.

[khk-globus]

I am not quite sure the side-channel conversations "that have been had" here, but I'll make the observation here that pragmatically, no one is using this interface — in no small part because it's not a complete solution: register_function is not what people come to Compute to do, they come for submit()sion of tasks.

In other words, yes this may be a breaking change, but it also will affect ~no one. "If a tree falls in the forest and no one is around ..."

That all said, I have no dog in this race. "Just observing from my armchair."

[LeiGlobus]

Yes a breaking change but possibly not yet used by anyone (other than during development), and leaving it in will introduce confusion (until removed), so not quite zero-cost.

I'm more in favor of cleanliness as opposed to backwards compatibility due to the timing of the various changes here, but ok with either way and would leave the decision to Reid.

[sirosen]

Just to make sure this is 100% clear to contributors:
The policy which I've generally insisted on, in my tenure as a maintainer of this library, is that no matter how new something is we keep it backwards compatible absent a truly pressing reason to break compatibility.

As a useful example, AuthClient.create_policy was introduced in one release and we immediately noticed a problem with it's signature. We iterated through 3 methodologies to fix it, and eventually landed on this PR, which is a fairly sophisticated bit of compatibility code. This was released in the release immediately subsequent to the introduction of AuthClient.create_policy and will persist until version 4 is released.

i.e. The bar is set very high and we aim not merely for "good" but for exemplary library behavior.

In this case, I allowed myself to be swayed from my typical stance in a discussion which included only a small subset of the team. I actually think I made an error in judgement by not taking this back to the entire team to discuss before giving Reid the go-ahead to implement in this way -- I was too enthusiastic about getting good quality contributions. We're going to rectify that by discussing amongst the SDK maintainers, as soon as everyone is available, and I expect we'll put a follow-up in this thread after that.

[rjmello]

The goal of changing the parameter name is to establish a consistent naming pattern for unstructured data parameters that matches other services. However, if this is an issue, we can leave function_data and simply remove the ComputeFunctionDocument type hint.

[sirosen]

@rjmello, yes, we just agreed that the best path here in the short term is to rename back to function_data. It's the pragmatic next-step, and it doesn't preclude renaming the parameter later if we really want to (with, potentially, a compatibility shim).

I'm dropping in a suggestion to do this right now.
If you punch those through (or, if you prefer the history it provides, revert or reset your branch to remove the last commit), we can approve and merge.

---

Popping back up a level, we haven't always had perfect agreement about when it is or is not okay to bend backwards compatibility rules. Sure, this is trivial, but the unfortunate thing is that it happened to hit precisely on something which we've disagreed about internally in the past.
There are some basic takeaways here:

• ⚔️ if we think a contribution is going near "disputed ground", we'll try let you know early so that it won't feel like you've walked into a trap
• 🔀 we'll try to catch this sort of issue early so that we can give you better answers at the outset -- as opposed to an early meeting that points you Caradhras and a PR review which points you towards Moria

I'm introspecting a bit here and feel that I made a mistake. By presenting a view in past discussions ("don't rename that parameter, that's a breaking change") which I then didn't present this morning, I created disharmony.
Really sorry about that! I'll try to make sure to stay more consistent or at least signal when (per above) we might enter some area which needs further discussion.

[rjmello]

No problem. I don't foresee us changing the name in the future, when ComputeClient has >0 users, so we'll accept the outlier parameter name.

[sirosen]

@rjmello, yes, we just agreed that the best path here in the short term is to rename back to function_data. It's the pragmatic next-step, and it doesn't preclude renaming the parameter later if we really want to (with, potentially, a compatibility shim).

I'm dropping in a suggestion to do this right now.
If you punch those through (or, if you prefer the history it provides, revert or reset your branch to remove the last commit), we can approve and merge.

---

Popping back up a level, we haven't always had perfect agreement about when it is or is not okay to bend backwards compatibility rules. Sure, this is trivial, but the unfortunate thing is that it happened to hit precisely on something which we've disagreed about internally in the past.
There are some basic takeaways here:

• ⚔️ if we think a contribution is going near "disputed ground", we'll try let you know early so that it won't feel like you've walked into a trap
• 🔀 we'll try to catch this sort of issue early so that we can give you better answers at the outset -- as opposed to an early meeting that points you Caradhras and a PR review which points you towards Moria

I'm introspecting a bit here and feel that I made a mistake. By presenting a view in past discussions ("don't rename that parameter, that's a breaking change") which I then didn't present this morning, I created disharmony.
Really sorry about that! I'll try to make sure to stay more consistent or at least signal when (per above) we might enter some area which needs further discussion.

[kurtmckee]

Since it's after hours I'll wait for @sirosen to weigh in but this looks good to me!

[sirosen]

Yep, all clear!

One last time, thanks to the team for being willing to take a pause to sort this out, in spite of it being such a minor issue. I regret how chaotic it was, especially for a relatively new contributor to the repo, but I think we're well positioned to avoid such snags going forward.