Page MenuHomePhabricator

Revisit best practices and code support for local & global keys
Closed, ResolvedPublic

Description

It appears we lack a common understanding of where local and global keys can appropriately occur, and also lack code consistency in the support for local and global keys. This can cause unexpected bugs and time spent diagnosing and fixing them. It can also require time for each developer to investigate and understand what needs to be supported. We need greater clarity and precise documentation about this.

Illustration: Recently it was necessary to modify a front end JavaScript utility (getValueFromCanonicalZMap) that handles ZTypedPair instances, so as to allow for either local (K1 and K2) or global (Z882K1 and Z882K2) keys. (I'm referring to the keys for "first element" and "second element", not keys that are used inside the Z1K1.) The ZPair type processed by this utility is transient, not persistent.

This quote from function model page indicates that a local key is appropriate in this setting: "The main use case for Local Keys is when a Z8/Function or Z4/Type is being created on the fly, and thus cannot have Global Keys because the created Z8/Function or Z4/Type itself is not persistent." It was also necessary to do new work on ZObjectFactory::create() because it expected global keys. Currently, it still changes local keys in the input (K1 and K2, which were placed there by the orchestrator, and are appropriately local) to global keys in its output (Z882K1 and Z882K2), at least in some cases. Let's consider whether that behavior should change.

Questions: Should we move away from this ambiguity, so that eventually we only need to support local keys in this situation? More generally, should we have a completely unique canonical form for each ZObject, or allow for certain variants in the canonical form?

See also: T313880 and its patches.

Note: regarding getValueFromCanonicalZMap, it's also present in function-schemata, and currently only supports local keys. It's my impression that function-schemata functions generally support less variation with respect to local versus global keys.
Note: See also engineering meeting notes for August 4, 2022.

Event Timeline

This topic also came up while working on scoping in the orchestrator. It turns out there are several different usages of local keys and subtle differences between them. I went ahead and summarized my current understanding in this doc, I hope it will help the team when tackling this subject: https://docs.google.com/document/d/1yGnvukO8hjOP3r7zdCjWNG1mQBQdICHyEQscGjC_rXQ/edit

As mentioned in today's eng meeting, "Z882K1" should probably not be accepted because the type is a transient type generated by a function. This is also touched up in the doc,

Geno wrote the following:

https://docs.google.com/document/d/1ZthdIKLGY0QPpqiiIlD2Wuua2FtXr8sGDjNZ4wjNCD0

which proposes a fix to the getDefinition method in ZTypedPair.php, which other team members have also agreed to. I made that fix locally, and it introduced 6 failures in WikiLambda PHP unit tests. Didn't have time to investigate those failures yet, so the fix has not been committed.

Change 833139 had a related patch set uploaded (by David Martin; author: David Martin):

[mediawiki/extensions/WikiLambda@master] Correct element key names in ZTypedPair

https://gerrit.wikimedia.org/r/833139

Change 833139 merged by jenkins-bot:

[mediawiki/extensions/WikiLambda@master] Correct element key names in ZTypedPair

https://gerrit.wikimedia.org/r/833139

Remaining steps:

  • Back out https://gerrit.wikimedia.org/r/817833; it shouldn't be needed now that ZTypedPair has been updated. It makes ZObjectFactory more lax (unnecessarily, I believe) while checking what keys are available in the object. - DONE
  • Make sure appropriate use of local and global keys is clearly documented.
  • Make sure we have consensus about the documentation.

Change 835680 had a related patch set uploaded (by David Martin; author: David Martin):

[mediawiki/extensions/WikiLambda@master] Revert patch 817833 changes to ZObjectFactory.php

https://gerrit.wikimedia.org/r/835680

Change 835680 merged by jenkins-bot:

[mediawiki/extensions/WikiLambda@master] Follow-up d0772d3: Revert changes to ZObjectFactory.php

https://gerrit.wikimedia.org/r/835680