Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix asdftool diff output for arrays in list #1672

Merged
merged 1 commit into from
Oct 30, 2023

Conversation

braingram
Copy link
Contributor

@braingram braingram commented Oct 27, 2023

#1652 introduced a bug where running asdftool diff on files that contain an array nested within a list would fail. See the following traceback:

FAILED test/test_diff.py::test_diff_asdf - assert 'Traceback (m...yNode found\n' == '        ndar...     GRISMC\n'
  + Traceback (most recent call last):
  +   File "/home/runner/micromamba/envs/crds-testing/bin/asdftool", line 10, in <module>
  +     sys.exit(main())
  +   File "/home/runner/micromamba/envs/crds-testing/lib/python3.10/site-packages/asdf/commands/main.py", line 80, in main
  +     sys.exit(main_from_args(args))
  +   File "/home/runner/micromamba/envs/crds-testing/lib/python3.10/site-packages/asdf/commands/main.py", line 63, in main_from_args
  +     result = args.func(args)
  +   File "/home/runner/micromamba/envs/crds-testing/lib/python3.10/site-packages/asdf/commands/diff.py", line 98, in run
  +     return diff(args.filenames, args.minimal, ignore=args.ignore)
  +   File "/home/runner/micromamba/envs/crds-testing/lib/python3.10/site-packages/asdf/commands/diff.py", line 371, in diff
  +     compare_trees(diff_ctx, asdf0.tree, asdf1.tree)
  +   File "/home/runner/micromamba/envs/crds-testing/lib/python3.10/site-packages/asdf/commands/diff.py", line 321, in compare_trees
  +     compare_dicts(diff_ctx, tree0, tree1, keys)
  +   File "/home/runner/micromamba/envs/crds-testing/lib/python3.10/site-packages/asdf/commands/diff.py", line 304, in compare_dicts
  +     compare_trees(diff_ctx, obj0, obj1, keys=[*keys, key])
  +   File "/home/runner/micromamba/envs/crds-testing/lib/python3.10/site-packages/asdf/commands/diff.py", line 325, in compare_trees
  +     compare_trees(diff_ctx, obj0, obj1, [*keys, key])
  +   File "/home/runner/micromamba/envs/crds-testing/lib/python3.10/site-packages/asdf/commands/diff.py", line 321, in compare_trees
  +     compare_dicts(diff_ctx, tree0, tree1, keys)
  +   File "/home/runner/micromamba/envs/crds-testing/lib/python3.10/site-packages/asdf/commands/diff.py", line 304, in compare_dicts
  +     compare_trees(diff_ctx, obj0, obj1, keys=[*keys, key])
  +   File "/home/runner/micromamba/envs/crds-testing/lib/python3.10/site-packages/asdf/commands/diff.py", line 319, in compare_trees
  +     compare_ndarrays(diff_ctx, tree0, tree1, keys)
  +   File "/home/runner/micromamba/envs/crds-testing/lib/python3.10/site-packages/asdf/commands/diff.py", line 281, in compare_ndarrays
  +     msg = f"ndarrays at \"{'.'.join(keys)}\" differ by {human_list(differences)}"
  + TypeError: sequence item 1: expected str instance, ArrayNode found

This PR fixes (and adds a test for) the bug by modifying asdftool diff to use print_in_tree (which handles the ArrayNode instance keys). This has the positive side-effect of allowing the array difference output to be nested within the diff tree. Using two of the test data files and running asdftool diff ndarray0.asdf ndarray1.asdf prior to this PR outputs:

    ndarrays at "a" differ by contents

With this PR the output is more consistent with the format of other differences (shown nested within the diff tree):

tree:
  a:
>   ndarrays differ by contents
<   ndarrays differ by contents

The above issue was revealed when jwst increased it's upper asdf pin which was constraining crds tests to only test against asdf less than 3. It turns out that crds is directly using the asdftool diff output:
https://github.com/spacetelescope/crds/blob/d5d92a1c937d13f4a80360b9de71935e835e0d40/crds/diff.py#L368
and after this PR crds tests will need to be updated as they are still expecting the old (pre 3) output:
https://github.com/spacetelescope/crds/blob/d5d92a1c937d13f4a80360b9de71935e835e0d40/test/test_diff.py#L181

A test PR is open for crds that uses asdf from the source branch for this PR and includes a number of other asdf 3.0 fixes (and currently has passing tests showing that the fix in this PR is sufficient for that portion of the crds/asdf-3.0 issues). spacetelescope/crds#1004

Copy link
Contributor

@eslavich eslavich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@braingram braingram merged commit 5cadc9f into asdf-format:main Oct 30, 2023
54 of 64 checks passed
@braingram braingram deleted the diff_regression branch October 30, 2023 10:31
braingram added a commit to braingram/asdf that referenced this pull request Oct 30, 2023
fix asdftool diff output for arrays in list
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants