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

Feature/daphnelib python pkg #649

Merged
merged 20 commits into from
Apr 23, 2024

Conversation

m-birke
Copy link
Collaborator

@m-birke m-birke commented Jan 19, 2024

Building the Python DaphneLib as a package
Now independent from CWD of python3 cmd execution when running a DLIB script
Build artifact producable which can be distributed
(potentially can be separated into different git repo now very easily)

I had to touch 90 files for this PR which is a lot, let me point reviewers here to the important changes:

  1. I moved the python source code to a daphne subdir to actually have a python package with the name 'daphne'
  2. The imports are changed to 'from daphne.... import ...'
  3. The path to the libdaphnelib.so is acquired via a env var which specifies the dir where all the necessary .so files are located
  4. The daphnelib (cpp) gets the path to the liballkernels.so as a parameter to be independent from PWD
  5. Within the src/api/python dir Python project management files are placed
  6. I changed the tmp path to /tmp/DaphneLib because if the Python 'daphne' is installed as a package we dont know the location of the files and hence cant assume the a file related path is writable

Main issue solved in source code: removal of hardcoded paths

There are more improvements possible but for this PR it is enough

@pdamme pdamme self-requested a review January 24, 2024 21:21
@pdamme
Copy link
Collaborator

pdamme commented Jan 24, 2024

Thanks for this contribution, @m-birke. Looks like this will simplify using DaphneLib a lot! I will look into it in detail later this week.

@pdamme pdamme force-pushed the feature/daphnelib-python-pkg branch from 3a50a7e to 4297474 Compare April 23, 2024 16:05
- Docs mention that/how DaphneLib can be used from source without installing the package.
- tmpdaphne.daphne is stored in /tmp/DaphneLib, not in the pwd.
- Updated the version number of the Python package to the most recent DAPHNE release version number.
- Fixed some typos.
- Added missing license headers.
@pdamme pdamme force-pushed the feature/daphnelib-python-pkg branch from 4297474 to cd6496a Compare April 23, 2024 16:07
@pdamme
Copy link
Collaborator

pdamme commented Apr 23, 2024

Hi @m-birke, sorry for the delay... This contribution looks very good to me. I've just polished a few little things (see the commit I added), such as:

  • docs briefly mention that/how DaphneLib can be used from source without installing the package (which we still do in test.sh and run-python.sh)
  • tmpdaphne.daphne is also stored in /tmp/DaphneLib/, not in the pwd (good to prevent cluttering with these files, now that DaphneLib can be run from any directory)
  • updated the version number of the Python package to the most recent DAPHNE release version number (as we discussed in some meeting a while ago)

I think it would be great to add the installation and little test use of DaphneLib to our GitHub actions to always test if it still works as described in the docs. I would recommend a package installation from the already cloned source then, since installing from the GitHub repo seems to clone the LLVM submodule, which is quite huge.

Copy link
Collaborator

@pdamme pdamme left a comment

Choose a reason for hiding this comment

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

See the above comment for my review.

@pdamme pdamme merged commit 25f2a3c into daphne-eu:main Apr 23, 2024
2 checks passed
@m-birke
Copy link
Collaborator Author

m-birke commented Apr 26, 2024

Hi @pdamme

thanks for the review and the additional fixes. Especially saving "tmpdaphne.daphne" to the new tmp dir as well is important

takeaway for me: add github workflow for actually building and testing the package (or general python related ci/cd), i will create an issue for this

KR

@m-birke m-birke deleted the feature/daphnelib-python-pkg branch November 12, 2024 09:43
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.

2 participants