Clear warnings for each file in codemod cli #1184

kiri11 · 2024-08-03T22:17:04Z

Summary

Original idea belongs to @jschavesr. I am just resubmitting his abandoned PR.
Fixes the issue: #1183

Context warning messages were not being cleaned between files, so warnings from one files could be passed to other files in the context. Because of that warnings were being shown wrongly in the summary and the total number of warnings was wrong as well.

I also encountered another error in Windows CI. Seems like codemodding small files happens faster than the timer counts more than zero (probably less precision on Windows). So fixing that, too.

Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "D:\a\LibCST\LibCST\libcst\tool.py", line 689, in <module>
    main(os.environ.get("LIBCST_TOOL_COMMAND_NAME", "libcst.tool"), sys.argv[1:])
  File "D:\a\LibCST\LibCST\libcst\tool.py", line 684, in main
    return lookup.get(args.action or None, _invalid_command)(proc_name, command_args)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "D:\a\LibCST\LibCST\libcst\tool.py", line 420, in _codemod_impl
    result = parallel_exec_transform_with_prettyprint(
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "D:\a\LibCST\LibCST\libcst\codemod\_cli.py", line 659, in parallel_exec_transform_with_prettyprint
    progress.print(successes + failures + skips)
  File "D:\a\LibCST\LibCST\libcst\codemod\_cli.py", line 398, in print
    f"{self.ERASE_CURRENT_LINE}{self._human_seconds(elapsed_time)} {percent:.{self.pretty_precision}f}% complete, {self.estimate_completion(elapsed_time, finished, left)} estimated for {left} files to go...",
                                                                                                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "D:\a\LibCST\LibCST\libcst\codemod\_cli.py", line 430, in estimate_completion
    fps = files_finished / elapsed_seconds
          ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
ZeroDivisionError: float division by zero

Test Plan

Before:

Running convert_format_to_fstring.ConvertFormatStringCommand on three files with identical contents:

def baz() -> str:
    return "{}: {}".format(*baz)

All three are the same, so they should get one warning each.
But instead every following file gets more warnings copied from previous files:

LibCST> python -m libcst.tool codemod convert_format_to_fstring.ConvertFormatStringCommand tmp
Calculating full-repo metadata...
Executing codemod...
Codemodding tmp\mod1.py
WARNING: Unsupported field_name 0 in format() call
Successfully codemodded tmp\mod1.py with warnings

Codemodding tmp\mod2.py
WARNING: Unsupported field_name 0 in format() call
WARNING: Unsupported field_name 0 in format() call
Successfully codemodded tmp\mod2.py with warnings

Codemodding tmp\mod3.py
WARNING: Unsupported field_name 0 in format() call
WARNING: Unsupported field_name 0 in format() call
WARNING: Unsupported field_name 0 in format() call
Successfully codemodded tmp\mod3.py with warnings

Finished codemodding 3 files!
 - Transformed 3 files successfully.
 - Skipped 0 files.
 - Failed to codemod 0 files.
 - 6 warnings were generated.

The new test fails:

FAIL: test_warning_messages_several_files (libcst.codemod.tests.test_codemod_cli.TestCodemodCLI.test_warning_messages_several_files)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "libcst\codemod\tests\test_codemod_cli.py", line 92, in test_warning_messages_several_files
    self.assertIn(
AssertionError: '- 3 warnings were generated.' not found in "Calculating full-repo metadata... ... \n - 6 warnings were generated.\n"

After:

Each file generates one warning correctly:

LibCST> python -m libcst.tool codemod convert_format_to_fstring.ConvertFormatStringCommand tmp
Calculating full-repo metadata...
Executing codemod...
Codemodding tmp\mod1.py
WARNING: Unsupported field_name 0 in format() call
Successfully codemodded tmp\mod1.py with warnings

Codemodding tmp\mod2.py
WARNING: Unsupported field_name 0 in format() call
Successfully codemodded tmp\mod2.py with warnings

Codemodding tmp\mod3.py
WARNING: Unsupported field_name 0 in format() call
Successfully codemodded tmp\mod3.py with warnings

Finished codemodding 3 files!
 - Transformed 3 files successfully.
 - Skipped 0 files.
 - Failed to codemod 0 files.
 - 3 warnings were generated.

And the new test passes.

When codemodding too fast

codecov · 2024-08-03T22:41:55Z

Codecov Report

Attention: Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.

Project coverage is 91.26%. Comparing base (56cd1f9) to head (71986bf).

Files	Patch %	Lines
libcst/codemod/_cli.py	57.14%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1184   +/-   ##
=======================================
  Coverage   91.26%   91.26%           
=======================================
  Files         261      261           
  Lines       26883    26898   +15     
=======================================
+ Hits        24535    24549   +14     
- Misses       2348     2349    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zsol · 2024-08-04T15:20:18Z

libcst/codemod/_cli.py

        # We do this after the fork so that a context that was initialized with
        # some defaults before calling parallel_exec_transform_with_prettyprint
        # will be updated per-file.
+        # Clean the warnings as well, otherwise they will be
+        # passed from the previous file
        transformer.context = replace(
            transformer.context,
            filename=filename,
            scratch=deepcopy(scratch),
+            warnings=[],


While this change will fix the issue, there's a deeper problem here. Apart from metadata_manager, every other field of context should be reset per file.
Concretely, I'm surprised we change filename here but not full_module_name or full_package_name.

Maybe we should make this explicit and have a global context that persists between files, and a local one that gets reset. Then this assignment would look something like:

try: mod_and_pkg = calculate_module_and_package(config.repo_root or ".", filename) mod_name = mod_and_pkg.name pkg_name = mod_and_pkg.package except ValueError as exc: print(f"Failed to determine module name for {filename}: {ex}", file=sys.stderr) mod_name = None pkg_name = None transformer.context = CodemodContext(scratch=deepcopy(scratch), filename=filename, full_module_name=mod_name, full_package_name=pkg_name, metadata_manager=transformer.context.metadata_manager)

Having said that, I'm happy to merge the PR as is because it's definitely an improvement over the current state. Let me know if you wanna roll this change into this PR or not

full_module_name or full_package_name are changed below, so currently context replaced twice. And if it isn't possible to determine the module/package name, it will persist from the previous file. The wrapper also persists. That would be changing in the new commit I'm submitting.

Before:

full_module_name/full_package_name persist if can't get them for the new file

wrapper persists across files

After:

full_module_name/full_package_name both set to None if can't figure them out.

wrapper is set to None on every file.

Is that okay to not set the wrapper?

Yep, it should be

Keep only context.metadata_manager Remove wrapper from context defaults on each file

zsol · 2024-08-05T21:42:08Z

Thanks!

Clean warnings for each file in comemod cli

b0dbe99

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 3, 2024

Fix ZeroDivisionError: float division by zero

4077ff0

When codemodding too fast

kiri11 marked this pull request as ready for review August 3, 2024 22:57

zsol approved these changes Aug 4, 2024

View reviewed changes

Recreate CodemodContext for each file

71986bf

Keep only context.metadata_manager Remove wrapper from context defaults on each file

zsol merged commit 45234f1 into Instagram:main Aug 5, 2024
21 of 22 checks passed

kiri11 changed the title ~~Clear warnings for each file in comemod cli~~ Clear warnings for each file in codemod cli Sep 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clear warnings for each file in codemod cli #1184

Clear warnings for each file in codemod cli #1184

kiri11 commented Aug 3, 2024 •

edited

Loading

codecov bot commented Aug 3, 2024 •

edited

Loading

zsol Aug 4, 2024

zsol Aug 4, 2024

kiri11 Aug 4, 2024

zsol Aug 5, 2024

zsol commented Aug 5, 2024

Clear warnings for each file in codemod cli #1184

Clear warnings for each file in codemod cli #1184

Conversation

kiri11 commented Aug 3, 2024 • edited Loading

Summary

Test Plan

Before:

After:

codecov bot commented Aug 3, 2024 • edited Loading

Codecov Report

zsol Aug 4, 2024

Choose a reason for hiding this comment

zsol Aug 4, 2024

Choose a reason for hiding this comment

kiri11 Aug 4, 2024

Choose a reason for hiding this comment

zsol Aug 5, 2024

Choose a reason for hiding this comment

zsol commented Aug 5, 2024

kiri11 commented Aug 3, 2024 •

edited

Loading

codecov bot commented Aug 3, 2024 •

edited

Loading