[TVMC] tune: Check if FILE exists (#10608)

[gromero]

Currently when a non-existing FILE is passed to 'tvmc tune' it throws
a traceback because a FileNotFoundError exception is not handled. Since
there is no need for such abrupt exit, and the trace can also confuse
users, this commit fixes it by checking if FILE indeed exists, kindly
informing the user about the non-existing FILE before exiting.

Signed-off-by: Gustavo Romero gustavo.romero@linaro.org

Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers by @ them in the pull request thread.

[Mousius]

Is it possible to add a unit test for this?

[gromero]

@Mousius I'm a sinner, I recognize. But seeking redemption I have added a test case also for tvmc compile which I have fixed recently.

Currently the CI is failing due to a TVMCException caught in test_tune_rpc_tracker_parsing test and since I was not able to reproduce it locally I've added a fix on my guess about the root cause. Let's see if it pleases the CI.

Thanks for the quick review!

[Mousius]

Thanks for adding the tests @gromero, I have a few suggestions to improve them:

• Create an explicit test for each part of tvmc tested
• Use a temporary directory outside of the tree to keep the tree itself tidy
• Split the file creation into pytest.fixtures which should mean we don't need the clean up logic in the test itself or the branching to figure out what to remove
• Use pytest.parameterize to generate the test cases rather than loops inside the tests

Something like:

@pytest.fixture
def missing_file(tmp_path):
  missing_file_name = "missing_file_as_invalid_input.tfite"
  yield missing_file_name

@pytest.fixture
def broken_symlink(tmp_path):
    broken_symlink = "broken_symlink_as_invalid_input.tflite"
    os.symlink("non_existing_file", tmp_path /  broken_symlink)
    yield broken_symlink
    os.unlink(tmp_path / broken_symlink)

@pytest.fixture
def fake_directory(tmp_path):
    dir_as_invalid = "dir_as_invalid_input.tflite"
    os.mkdir(tmp_path / dir_as_invalid)
    yield dir_as_invalid
    shutil.rmtree(tmp_path / dir_as_invalid)

@pytest.mark.parametrize("invalid_input", [missing_file, broken_symlink, fake_directory])
def test_tvmc_compile_invalid_input(capsys, missing_file, broken_symlink, fake_directory, invalid_input):
        compile_cmd = f"tvmc compile --target 'c' {invalid_input}"
        run_arg = compile_cmd.split(" ")[1:]

        _main(run_arg)

        captured = capsys.readouterr()
        expected_err = (
            f"Error: Input file '{invalid_input}' doesn't exist, "
            "is a broken symbolic link, or a directory.\n"
        )
        on_assert_error = f"'tvmc compile' failed to check invalid FILE: {invalid_input}"
        assert captured.err == expected_err, on_assert_error

What do you think? 😸

[gromero]

@Mousius Thanks for the suggestions! All of them are good improvements. I specially like the one for using a fixture + a generator for preparing the test environment and cleaning it up all at once. But there is a caveat: fixtures don't play well with @pytest.mark.parametrize. This is by design afaik.

In the code above missing_file, broken_symlink, and fake_directory won't reach out the scope of test_tvmc_compile_invalid_input() as str, but as a function, i.e. the returned value of the fixture is not passed to test_tvmc_compile_invalid_input , but instead the fixture itself is (pytest-dev/pytest#349). And unfortunately a fixture can't be called directly allowing something like:

@pytest.mark.parametrize("invalid_input", [missing_file(), broken_symlink(), fake_directory()])

There might be some other "magic" workaround, but that caveat is an example of pytest "shenanigans" that annoys me. So sometimes I tend to do more "direct" tests, although that for sure goes against the pytest paradigms...

Anyway, I think the best solution would be to use "pytest-lazy-fixture" to implement your suggestion (https://github.com/tvorog/pytest-lazy-fixture). This requires pytest-lazy-fixture to get installed into the CI images. I've pushed (to this PR) a working v2 using lazy_fixture that gives me the following result:

gromero@amd:~/git/tvm$ pytest -vvv tests/python/driver/tvmc/test_command_line.py 
[04:45:59] /home/gromero/git/tvm/src/target/target_kind.cc:163: Warning: Unable to detect CUDA version, default to "-arch=sm_20" instead
enabled targets: llvm
pytest marker: 
==================================================================================== test session starts =====================================================================================
platform linux -- Python 3.9.5, pytest-6.2.4, py-1.10.0, pluggy-0.13.1 -- /usr/bin/python3
cachedir: .pytest_cache
rootdir: /home/gromero/git/tvm
plugins: lazy-fixture-0.6.3
collected 7 items                                                                                                                                                                            

tests/python/driver/tvmc/test_command_line.py::test_tvmc_cl_workflow PASSED                                                                                                            [ 14%]
tests/python/driver/tvmc/test_command_line.py::test_tvmc_compile_file_check[missing_file] PASSED                                                                                       [ 28%]
tests/python/driver/tvmc/test_command_line.py::test_tvmc_compile_file_check[broken_symlink] PASSED                                                                                     [ 42%]
tests/python/driver/tvmc/test_command_line.py::test_tvmc_compile_file_check[fake_directory] PASSED                                                                                     [ 57%]
tests/python/driver/tvmc/test_command_line.py::test_tvmc_tune_file_check[missing_file] PASSED                                                                                          [ 71%]
tests/python/driver/tvmc/test_command_line.py::test_tvmc_tune_file_check[broken_symlink] PASSED                                                                                        [ 85%]
tests/python/driver/tvmc/test_command_line.py::test_tvmc_tune_file_check[fake_directory] PASSED                                                                                        [100%]

====================================================================================== warnings summary ======================================================================================
tests/python/driver/tvmc/test_command_line.py::test_tvmc_cl_workflow
  /home/gromero/.local/lib/python3.9/site-packages/tensorflow/python/autograph/impl/api.py:22: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
    import imp

tests/python/driver/tvmc/test_command_line.py::test_tvmc_cl_workflow
  /home/gromero/.local/lib/python3.9/site-packages/xgboost/training.py:18: UserWarning: Old style callback is deprecated.  See: https://xgboost.readthedocs.io/en/latest/python/callbacks.html
    warnings.warn(f'Old style callback is deprecated.  See: {link}', UserWarning)

-- Docs: https://docs.pytest.org/en/stable/warnings.html
=============================================================================== 7 passed, 2 warnings in 8.66s ================================================================================

But it will fail in the CI.

In the v2 I've also decided to keep a test_tvmc_compile_file_check and a test_tvmc_tune_file_check because in the future the FILE in tune can accept other inputs beyond a regular file and so the error message can check change and become different than in compile. I also avoided defining missing_file as a generator, so I defined it as a simple function since there's no env. preparation etc.

wdyt? Should we move on with using pytest-lazy-fixture?

[Mousius]

I had a quick read of that pytest thread, I think my assumptions around fixtures were with non-yield fixtures that can be invoked where-as here we want the full fixture life cycle. Looks like there's either pytest-cases or pytest-lazy-fixture here. pytest-cases is a bit more powerful and opinionated, so I think you're right in choosing pytest-lazy-fixture, it's simpler so should compose well into tvm.testing rather than tying us into the style of pytest-cases.

Let's move forwards with adding pytest-lazy-fixture into ubuntu_install_python_package.sh, and see what
people think. Are you ok with this being delayed a bit for that or do you want to merge the original, get the package installed and refactor afterwards?

CC @driazati @areusch @leandron for thoughts

[gromero]

I had a quick read of that pytest thread, I think my assumptions around fixtures were with non-yield fixtures that can be invoked where-as here we want the full fixture life cycle. Looks like there's either pytest-cases or pytest-lazy-fixture here. pytest-cases is a bit more powerful and opinionated, so I think you're right in choosing pytest-lazy-fixture, it's simpler so should compose well into tvm.testing rather than tying us into the style of pytest-cases.

Thanks for checking other possibilities. Yeah I agree, I had the same impression about pytest-cases. :)

Let's move forwards with adding pytest-lazy-fixture into ubuntu_install_python_package.sh, and see what people think. Are you ok with this being delayed a bit for that or do you want to merge the original, get the package installed and refactor afterwards?

CC @driazati @areusch @leandron for thoughts

@Mousius I'm totally fine with holding off this PR until we add pytest-lazy-fixture to the CI images. Let's also see what folks think about it. Thank you!

[driazati]

You could also use TemporaryDirectory manually instead of the pytest fixture if you don't want to wait

[gromero]

@areusch Cool. Thanks for the review! I'm proceeding to create a PR to add pytest-lazy-fixture to the install script and @Mousius will help me to update the CI images :)

[gromero]

@areusch @Mousius ubuntu_install_python_package.sh is updated in gromero@bac2859

@driazati @Mousius @leandron I'm wondering if I do need to create a PR to update the CI image(s) accordingly to https://github.com/apache/tvm/blob/main/docs/contribute/pull_request.rst#ci-environment ?

[Mousius]

@gromero you should be able to create an issue from the template here:
https://github.com/apache/tvm/issues/new?assignees=&labels=&template=ci-image.md&title=%5BCI+Image%5D+

If you get it started I can help fill out any additional values 😸

[gromero]

@Mousius Done: #10984 And yes, I'll need a hand for filling in the remaining values in this form, but I've put all know already there. Thanks.

[gromero]

Now that CI is updated, I'm re-triggering. Thanks folks!

[areusch]

@gromero please fix the merge conflict
@Mousius please take another look at this one

[gromero]

@Mousius Hi. This is ready for a final review (no changes tho since our last discussion [here] (#10865 (comment)). I only rebased to resolve a conflict) Could you please take another look and see if you're ok with the changes now that CI is green again? Thanks!

[Mousius]

Thanks for taking the time to work through this and rolling out a new way of organising tests @gromero! These tests look so much nicer 😸 !