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

python: Implement Python high level API in python #3938

Merged
merged 1 commit into from
Dec 29, 2023

Conversation

vicentebolea
Copy link
Collaborator

@vicentebolea vicentebolea commented Nov 22, 2023

Step of #3813

In this PR we move the implementation of File from C++/py11bind to pure Python. In addition to that we also add a High level python API that wraps the python bindings, this high level python API is needed to provide more pythonic supports and to connect the (File)Stream|FileReader with the other ADIOS2 objects (IO, adios, engine...)

  • The Python File class has been re-implemented in python in two different classes
    • Stream (for write and read modes)
    • FileReader (for ReadRandomAccess mode)
  • More member functions from the ADIOS2 core API has been exposed to the bindings.
  • Added several unit tests to test this new API.
  • Adds new linters/format (black|pylint|flake8) checks for the format CI build.
  • Updaded the readthedocs description for the Python high level API.
  • Moved the numpy dependency from the C++ interface to the Python API. This can hinder performance since if the user uses numpy we might incur in an extra memory copy. The reason to remove this was to easy the deployment of this library. @eisenhauer we can rollback this, I wonder what do you think about this.

@vicentebolea vicentebolea self-assigned this Nov 22, 2023
@vicentebolea vicentebolea marked this pull request as draft November 22, 2023 00:44
@vicentebolea vicentebolea force-pushed the add-new-python-api branch 6 times, most recently from d7f6a8b to a9edaf6 Compare November 23, 2023 23:04
@vicentebolea vicentebolea force-pushed the add-new-python-api branch 6 times, most recently from 3289b0c to 5e6831f Compare November 30, 2023 02:28
pyproject.toml Outdated Show resolved Hide resolved
@vicentebolea vicentebolea force-pushed the add-new-python-api branch 11 times, most recently from 42fb6ee to 3c362bb Compare December 15, 2023 18:42
@vicentebolea
Copy link
Collaborator Author

@dmitry-ganyushin ready for another round of review. I added pylint to the CI and I modify the code so that it satisfies most of the warnings, I disabled a few of them because since we use a C++ generated python module some of the checks do not work properly.

@vicentebolea vicentebolea force-pushed the add-new-python-api branch 2 times, most recently from c38260c to f69cb75 Compare December 22, 2023 16:58
@vicentebolea vicentebolea force-pushed the add-new-python-api branch 9 times, most recently from 433f0ff to 891d1ef Compare December 26, 2023 20:56
@vicentebolea vicentebolea changed the title python: rewrite Python high level API in python python: Implement Python high level API in python Dec 27, 2023
@vicentebolea vicentebolea force-pushed the add-new-python-api branch 4 times, most recently from a48e149 to 73d2c37 Compare December 27, 2023 02:46
@vicentebolea
Copy link
Collaborator Author

@eisenhauer please review this 👼

@eisenhauer
Copy link
Member

@eisenhauer please review this 👼

I only have a little bit of time this morning and there are a lot of changes, so probably won't get through this until the evening, but will do what I can.

@vicentebolea
Copy link
Collaborator Author

@eisenhauer please review this 👼

I only have a little bit of time this morning and there are a lot of changes, so probably won't get through this until the evening, but will do what I can.

Thanks! There has been several reviews from Scott (KW), Berk, and Dmitry, but I have made some changes since they reviewed it.

Copy link
Member

@eisenhauer eisenhauer left a comment

Choose a reason for hiding this comment

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

In py11Engine.cpp, MinBlocksInfo() returns a pointer, and if that pointer is non-null, the caller gains ownership to the created struct, so I think we need a delete of minBlocksInfo at the end of "if (minBlocksInfo)" in Engine::BlocksInfo.

@vicentebolea
Copy link
Collaborator Author

vicentebolea commented Dec 29, 2023 via email

@vicentebolea
Copy link
Collaborator Author

Good catch, find this resolved at the last push

On Thu, Dec 28, 2023 at 5:21 PM Greg Eisenhauer @.> wrote: @.* requested changes on this pull request. In py11Engine.cpp, MinBlocksInfo() returns a pointer, and if that pointer is non-null, the caller gains ownership to the created struct, so I think we need a delete of nBlocksInfo at the end of "if (mnBlocksInfo)" in Engine::BlocksInfo. — Reply to this email directly, view it on GitHub <#3938 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHFOFWZXDG4XISGXSIQKZDYLXWF3AVCNFSM6AAAAAA7VNKFSGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOOJYGQ3TINBWGA . You are receiving this because you were assigned.Message ID: @.***>
-- [image: logo shows large letter K with the word Kitware below it] http://www.kitware.com Vicente Bolea, MSc. Senior R&D Engineer

@eisenhauer

@vicentebolea vicentebolea merged commit 3f91c41 into ornladios:master Dec 29, 2023
39 checks passed
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