Upgrade typeguard, and add test that would have caught typeguard-related regression

[mmacpherson]

This refers to issue #2235.

The proximal issue is that you can't use a JSONType parameter via the Runner API with recent python versions.

That raises an error from the vendored-in typeguard library, which performs runtime type checks. That error arises in turn from updates to cpython, that made a previously-positional argument required to be specified as a keyword. Typeguard has since been patched to address this issue.

This PR aims to update the typeguard dependency, and to add a test that would have caught the issue when it first arose.

Adding the test is the more challenging problem. The current test runner invokes the API tests like so:

top_level_dict, run_level_dict = construct_arg_dicts_from_click_api()
runner = Runner(
                "test_flow.py", show_output=False, env=env, **top_level_dict
            )
result = runner.run(**run_level_dict)

To test an additional parameter via the path that has failed here, one would need somehow to get it into the run_level_dict.

I'm proposing here to augment the run_level_dict parameters via a RUNTIME_PARAMETERS environment variable that may be set in a test.

Running via:

 PYTHONPATH=`pwd`/../../ python run_tests.py --debug --contexts dev-local --graphs single-linear-step --tests BasicParameterTest

After setting RUNTIME_PARAMETERS now yields a test failure; the same "recursive_guard" issue under discussion.

With that in place, I patched typeguard, and that resolves the test failure.

I also ran PYTHONPATH=pwd/../../ python run_tests.py --debug --contexts dev-local --tests BasicParameterTest, and those passed as well. I might be wrong, but I'm expecting that the GH Actions would be relied upon to test other configurations.

They may well be a cleaner way to achieve test coverage of this code path, and I'm happy to modify/iterate if so. Hopefully this at least helps identify the key issues.

[mmacpherson]

Refactored this, adding a RuntimeParameters test aot modifying BasicParameters, and testing more parameter types there.

One weakness here is that these tests would all pass if the runtime parameters weren't applied. I thought a little about how to prove that they are being applied. For example, if you could detect from the test context whether we were running via the cli executor vs the api executor, then you could set a parameter differently in PARAMETERS vs RUNTIME_PARAMETERS and test that you've gotten the expected one. But I didn't find a good solution.

[romain-intel]

Thanks for the contribution!

I haven't looked in detail yet but a few high level comments:

• to update any vendored software (including typeguard), please update using the vendor.py script included (and change the file here: https://github.com/Netflix/metaflow/blob/master/metaflow/_vendor/vendor_any.txt to point to the version you are vendoring)
• for injecting the parameter into run_level_dict; I think a better approach may be to try to do json.loads on the parameters coming in and passing them as python object. Currently we do test for JSONType (ish) but they are all passed in as strings (iirc).

[mmacpherson]

Thanks kindly @romain-intel.

Pushed a couple commits:

• Uses the standard vendor path to upgrade typeguard (and typing_extensions, on which it depends).
• Undid the RUNTIME_PARAMETERS path, and tried what I think you meant.
• The way that I've done it only applies to JSONType parameters atm. If you want to test that other parameter types also work as intended when passed in to Runner.run() as python objects, we might refactor? Perhaps there's already a metaflow function that would unpack the default (string) values as encoded in MetaflowTest.PARAMETERS into the corresponding python type, that could be used?

[romain-intel]

Apart from using versions compatibile with 3.7 (partially), this looks good.

[romain-intel]

nit: to make the intent clearer, I would do something like: run_level_dict[param_name] = json.loads(run_level_dict[param_name] to make it clear that we are converting a string value to its JSON representation.

I don't know if we want to do that but we could also do one with string and one without json but I think that may be overkill. Thoughts @madhur-ob ?

[mmacpherson]

Thanks again.

I believe that edit wouldn't work atm, because the parameters -- i.e. the MetaflowTest.PARAMETERS -- aren't in run_level_dict; the present code snippet is adding them, well the JSONType subset of them.

For example, running this test:

PYTHONPATH=`pwd`/../../ python run_tests.py --debug --contexts dev-local --graphs single-linear-step --tests BasicParameterTest

with a print statement gives this as the contents run_level_dict just prior to the Runner.run(**run_level_dict) invocation -- absent the json.loads code:

run_level_dict={'max_workers': 50, 'max_num_splits': 10000, 'tags': ['刺身 means sashimi', 'multiple tags should be ok'], 'run_id_file': 'run-id'}

Where, with the present patch in place, it gives:

run_level_dict={'max_workers': 50, 'max_num_splits': 10000, 'tags': ['刺身 means sashimi', 'multiple tags should be ok'], 'run_id_file': 'run-id', 'json_param': {'a': [1, 2, 3]}}

[romain-intel]

We can't go that high for everything. 3.7 only goes up to 4.1.2. I'd recommend using that for 3.7 and then 4.4.0 for everything else (4.4.1 is only 3.9+). I believe this will fix your specific issue and keep the runner compatible with 3.7 as well. typing_extensions may have to be similarly updated. We have version specific vendored files for this.

[mmacpherson]

Thank you, this makes sense.

I pushed a commit that I believe addresses these points.

There had been no _vendor/version_v3_7.txt, so I created one. From testing by installing typeguard (4.1.2) into a 3.7 docker container, the importlib_metadata and zipp versions appear to be compatible with what's in _vendor/vendor_any.txt already, and then I set typing extensions to a typeguard+3.7 compatible version.

I also set typeguard to 4.4.0 in vendor_any.txt. That most-recent typing_extensions (4.12.2) claims compatibility back to 3.8, so I left it be.

[mmacpherson]

Thanks for running those tests @madhur-ob . It seems like it's two core issues that are causing the 18 current failures. I haven't had the time to investigate yet.