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[cartesian]: Fix serialize default behavior when Pickled property was not saved #1629

Conversation

FlorianDeconinck
Copy link
Contributor

Description

DaCe has a behavior of not saving properties if they have a default (newish behavior) which leads our 2-step process of library node caching to fail. Since we have decided to rely on pickling to cache out the content of the LibraryNode we respond to this change in DaCe by making sure default values pass to the pickle deserializer are returned plain

Requirements

  • All fixes and/or new features come with corresponding tests.
  • Important design decisions have been documented in the approriate ADR inside the docs/development/ADRs/ folder.

@FlorianDeconinck
Copy link
Contributor Author

Poke @romanc

@edopao
Copy link
Contributor

edopao commented Sep 4, 2024

It seems related to the change I made to the test code tests/cartesian_tests/integration_tests/multi_feature_tests/test_dace_parsing.py when I upgraded the DaCe package version. Is it possible that with your fix we can undo my change?

Copy link
Contributor

@romanc romanc left a comment

Choose a reason for hiding this comment

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

Looks good to me. Looking forward to not having to nuke my cache anymore all the time 🥳

Remove previous test fix for dace parsing
@FlorianDeconinck
Copy link
Contributor Author

It seems related to the change I made to the test code tests/cartesian_tests/integration_tests/multi_feature_tests/test_dace_parsing.py when I upgraded the DaCe package version. Is it possible that with your fix we can undo my change?

Should be, pushing code now

Copy link
Contributor

@twicki twicki left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Co-authored-by: Roman Cattaneo <romanc@users.noreply.github.com>
@havogt havogt requested a review from edopao September 5, 2024 11:01
Copy link
Contributor

@edopao edopao left a comment

Choose a reason for hiding this comment

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

Thanks @FlorianDeconinck for addressing my comment. LGTM

@FlorianDeconinck FlorianDeconinck merged commit 20082e9 into GridTools:main Sep 5, 2024
31 checks passed
@FlorianDeconinck FlorianDeconinck deleted the cartesian/fix/pickle_unserialize_behavior branch September 5, 2024 13:15
edopao pushed a commit to edopao/gt4py that referenced this pull request Sep 6, 2024
…was not saved (GridTools#1629)

## Description

DaCe has a behavior of _not_ saving properties if they have a default
(newish behavior) which leads our 2-step process of library node caching
to fail. Since we have decided to rely on pickling to cache out the
content of the LibraryNode we respond to this change in DaCe by making
sure default values pass to the pickle deserializer are returned plain

## Requirements

- [ ] All fixes and/or new features come with corresponding tests.
- [ ] Important design decisions have been documented in the approriate
ADR inside the [docs/development/ADRs/](docs/development/ADRs/Index.md)
folder.

---------

Co-authored-by: Florian Deconinck <florian.deconinck@gmail.com>
Co-authored-by: Roman Cattaneo <romanc@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants