Dummy inline arrays in asdf.util and weldxfile #469

marscher · 2021-08-06T14:20:10Z

Changes

The dummy block manager impl was replaced with a much simpler patching of the block creating function inside asdf. The replaced function does not allocate a block, but replaces the array with an empty list. With the additional asdf write option all_array_storage="internal" we achieve zero additional output memory for array serialization. This is being checked with a test.
The display_header function takes a (deep) copy of the Asdf tree, but without copying numpy arrays. This approach avoids side effects due manipulating the tree internally.
Added pytest options to use WeldxFile inside the read/write_buffer functions used in the testsuite. This increases the testing coverage of the class. Added a GH actions job to check this new configuration on Linux, py3.8 soley.

Checks

updated CHANGELOG.md
updated tests

run memory test on osx again, do not garbage collect

codecov · 2021-08-06T14:26:56Z

Codecov Report

Merging #469 (042a436) into master (afb3340) will increase coverage by 0.01%.
The diff coverage is 91.48%.

@@            Coverage Diff             @@
##           master     #469      +/-   ##
==========================================
+ Coverage   96.84%   96.86%   +0.01%     
==========================================
  Files          89       89              
  Lines        5550     5547       -3     
==========================================
- Hits         5375     5373       -2     
+ Misses        175      174       -1

Impacted Files	Coverage Δ
weldx/util.py	`99.18% <ø> (+0.81%)`	⬆️
weldx/asdf/util.py	`84.88% <87.50%> (+0.05%)`	⬆️
weldx/asdf/file.py	`97.43% <100.00%> (+0.54%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update afb3340...042a436. Read the comment docs.

marscher · 2021-08-06T14:56:15Z

osx is still fucked up (most likely because it is a fucked up os). Linux and Windows do no need more than 10% memory now during serialization. I've patched the copy behavior of ndarrays, because asdf always deepcopies the tree.

CagtayFabry · 2021-08-09T08:36:13Z

nice, really liking this solution
can we skip this specific test only for osx?

vhirtham · 2021-08-09T09:14:57Z

osx is still fucked up (most likely because it is a fucked up os). Linux and Windows do no need more than 10% memory now during serialization. I've patched the copy behavior of ndarrays, because asdf always deepcopies the tree.

        before = get_mem_info()
        fn = tempfile.mktemp(suffix=".wx", dir=tmpdir)
        with WeldxFile(mode=mode) as fh:
            fh["x"] = large_array
            fh.show_asdf_header(use_widgets=False, _interactive=False)
            fh.write_to(fn)

        after = get_mem_info()

Didn't really try to understand the whole test, but can't we move the second line out of the memcheck block? If we only want to check the memory footprint of show_asdf_header, we should remove as many possibly allocating commands as possible. Don't know what tempfile does under the hood, but maybe the fail on osx is not caused by show_asdf_header but one of the other involved lines. Just roughly followed this one, so sorry if I missed something.

can we skip this specific test only for osx?

I would want to understand why it is failing before disabling it. Disabling a failing test just lets me ask: why do we have the test in the first place if it isn't that important.

marscher · 2021-08-09T13:11:08Z

@vhirtham tempfile checks for an unused filename in $TEMPDIR and returns it. But the writeto method could involve larger memory footprints (e.g. buffers are kept in the background for whatever reason). I followed your advice and measure only the memory during show_asdf_header. And you are right about the test philosophy :) Still, osx sucks!

vhirtham · 2021-08-09T13:51:41Z

@vhirtham tempfile checks for an unused filename in $TEMPDIR and returns it. But the writeto method could involve larger memory footprints (e.g. buffers are kept in the background for whatever reason). I followed your advice and measure only the memory during show_asdf_header. And you are right about the test philosophy :) Still, osx sucks!

Glad to hear that it worked. 😉

…e WeldxFile

This reverts commit 98c5664.

CagtayFabry · 2021-08-12T07:06:42Z

In case there are still issues with osx I prefer to mark the test as xfail on that platform for now, see https://docs.pytest.org/en/latest/how-to/skipping.html#condition-parameter

CagtayFabry · 2021-08-14T11:44:50Z

I have changed the non interactive header output to just show the yaml string again for testing @marscher e29052f

marscher · 2021-08-16T11:35:53Z

I have changed the non interactive header output to just show the yaml string again for testing @marscher e29052f

I do not understand why pytest stdout capturing does not work for the print statement in asdf.info(). I also thought that the cluttering of the test output was quite annoying. But we should not change the implementation/desired output just because the test tool has a hick up, right?

CagtayFabry · 2021-08-16T12:06:58Z

I have changed the non interactive header output to just show the yaml string again for testing @marscher e29052f

I do not understand why pytest stdout capturing does not work for the print statement in asdf.info(). I also thought that the cluttering of the test output was quite annoying. But we should not change the implementation/desired output just because the test tool has a hick up, right?

there were some (probably unrelated issues)
however looking into it I actually like the pure yaml output also in a console because it shows the tag definitions etc and all nested elements. In addition asdf.info seems to do some kind of tag resolving to classes which is not really needed and imo can complicate things
However since the change is very simple we can revert back anytime?

CagtayFabry · 2021-08-16T12:07:41Z

On another note can you explain the idea behind the _USE_WELDX_FILE flag?

marscher · 2021-08-19T18:54:49Z

On another note can you explain the idea behind the _USE_WELDX_FILE flag?

The debug utility functions read_buffer and write_buffer will use WeldxFile internally, if the flag is set. Since these util functions are used extensively in the test suite, it increases the testing coverage of WeldxFile. There are two pytest flags to toggle this flag. I've also added a single pytest job to the matrix (just on Linux) to run this configuration.

# Conflicts: # weldx/asdf/file.py # weldx/asdf/util.py # weldx/tests/asdf_tests/test_weldx_file.py

pep8speaks · 2021-08-19T19:23:38Z

Hello @marscher! Thanks for updating this PR.

In the file weldx/asdf/util.py:

Line 1:5: E999 SyntaxError: invalid syntax

Comment last updated at 2021-08-20 12:04:57 UTC

.github/workflows/pytest.yml

This reverts commit 64e0c5afb5088ece8d7dc9761f6806b7545cc231.

marscher added 3 commits August 6, 2021 13:13

added dummy inline arrays to write_buffer

52319d1

fix+test

5f298ec

fix memory issue in WeldxFile.display_header by not deepcopying ndarray.

fdffc6d

run memory test on osx again, do not garbage collect

measure memory only for show_asdf_header

89af78f

marscher and others added 6 commits August 9, 2021 16:01

added pytest args to dynamically switch read/write_buffer funcs to us…

6608837

…e WeldxFile

add to matrix

4cc4a48

typo

a2c4b8a

amend changelog [ci skip]

98c5664

Revert "amend changelog [ci skip]"

67a076f

This reverts commit 98c5664.

Merge branch 'master' into dummy_inline_arrays

4167315

CagtayFabry assigned marscher Aug 12, 2021

CagtayFabry added file weldx file handling (WeldxFile) ASDF everything ASDF related (python + schemas) labels Aug 12, 2021

dont use asdf.info for static header output

e29052f

marscher added 3 commits August 19, 2021 20:36

fix debug util by passing correct array_storage args

3a2a683

invoke copy of filehandle not std api

ecc5db7

cleanup

ed0844a

Merge remote-tracking branch 'origin/master' into dummy_inline_arrays

371be95

# Conflicts: # weldx/asdf/file.py # weldx/asdf/util.py # weldx/tests/asdf_tests/test_weldx_file.py

updated changelog

059e7d4

formatting

f48b580

marscher marked this pull request as ready for review August 19, 2021 19:39

marscher requested a review from CagtayFabry August 19, 2021 20:00

CagtayFabry reviewed Aug 20, 2021

View reviewed changes

.github/workflows/pytest.yml Outdated Show resolved Hide resolved

Update .github/workflows/pytest.yml

208b181

CagtayFabry reviewed Aug 20, 2021

View reviewed changes

.github/workflows/pytest.yml Outdated Show resolved Hide resolved

Update .github/workflows/pytest.yml

0393c0b

CagtayFabry self-requested a review August 20, 2021 06:44

CagtayFabry approved these changes Aug 20, 2021

View reviewed changes

marscher added 6 commits August 20, 2021 13:46

fix gas

2ae3f93

Revert "fix gas"

6c9fb2f

This reverts commit 64e0c5afb5088ece8d7dc9761f6806b7545cc231.

inherit docstrings weldxfile

b770deb

f

85aaadf

remove extensions (again)

8379d8f

black

042a436

marscher merged commit fe0f622 into BAMWelDX:master Aug 20, 2021

marscher deleted the dummy_inline_arrays branch August 20, 2021 12:21

This was referenced May 4, 2023

ASDF unable to read empty inline array. asdf-format/asdf#1538

Closed

weldx.asdf.util.write_buffer produces invalid yaml for empty inline ndarrays #874

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dummy inline arrays in asdf.util and weldxfile #469

Dummy inline arrays in asdf.util and weldxfile #469

marscher commented Aug 6, 2021 •

edited

Loading

codecov bot commented Aug 6, 2021 •

edited

Loading

marscher commented Aug 6, 2021

CagtayFabry commented Aug 9, 2021

vhirtham commented Aug 9, 2021

marscher commented Aug 9, 2021

vhirtham commented Aug 9, 2021

CagtayFabry commented Aug 12, 2021

CagtayFabry commented Aug 14, 2021

marscher commented Aug 16, 2021

CagtayFabry commented Aug 16, 2021

CagtayFabry commented Aug 16, 2021

marscher commented Aug 19, 2021

pep8speaks commented Aug 19, 2021 •

edited

Loading

Dummy inline arrays in asdf.util and weldxfile #469

Dummy inline arrays in asdf.util and weldxfile #469

Conversation

marscher commented Aug 6, 2021 • edited Loading

Changes

Checks

codecov bot commented Aug 6, 2021 • edited Loading

Codecov Report

marscher commented Aug 6, 2021

CagtayFabry commented Aug 9, 2021

vhirtham commented Aug 9, 2021

marscher commented Aug 9, 2021

vhirtham commented Aug 9, 2021

CagtayFabry commented Aug 12, 2021

CagtayFabry commented Aug 14, 2021

marscher commented Aug 16, 2021

CagtayFabry commented Aug 16, 2021

CagtayFabry commented Aug 16, 2021

marscher commented Aug 19, 2021

pep8speaks commented Aug 19, 2021 • edited Loading

Comment last updated at 2021-08-20 12:04:57 UTC

marscher commented Aug 6, 2021 •

edited

Loading

codecov bot commented Aug 6, 2021 •

edited

Loading

pep8speaks commented Aug 19, 2021 •

edited

Loading