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

Use scikit build instead of setup.py #103

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Conversation

josephmckinsey
Copy link
Contributor

@josephmckinsey josephmckinsey commented Jul 24, 2024

  • Default behavior is to download helics from src and build.
  • If DOWNLOAD_BINARIES or CIBUILDWHEEL are defined, then it downloads the binaries from the repository.
  • There is not a way to download locally, even with an sdist. I couldn't quite figure out how to get scikit-build to use a local version of scikit build core along with its other dependencies.
  • Setting sdist always downloads helics-src.
  • Requires libzqm and boost to build from source. These are necessary for pytest, so I figured we should include them.
  • Switches to dynamic versioning. The setuptools-scm does not seem to be required, but @nightlark mentioned using the dynamic-version tag. What this means is that helics/_version.py has been deleted from git, and instead it is automatically generated from the latest git tag. This makes .github/workflows/update-helics.yml redundant.

Remaining items

  • Fix .github workflows to use cibuildwheel (and test resulting wheels)
  • Get offline builds working

@josephmckinsey josephmckinsey marked this pull request as ready for review August 6, 2024 14:49
@josephmckinsey josephmckinsey reopened this Aug 6, 2024
.github/workflows/docs.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
Comment on lines +10 to +14
workflow_run:
workflows: ["HELICS Version Update"]
branches: [main]
types:
- completed
Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of running it as part of the HELICS Version Update tag -- I wonder if instead the update workflow could call this workflow as a step, and then if this workflow succeeds + tests pass, the update workflow could create a new version tag/release in the repository that will trigger running this workflow to actually do the real PyPI build + release for the new version.

Reuseable workflow/workflow_call docs: https://docs.github.com/en/actions/sharing-automations/reusing-workflows#calling-a-reusable-workflow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does a new branch gets tagged right now? This doesn't happen in update helics version, right?

If I understand you correctly, you mean

  1. Update HELICS version gets run.
  2. Run tests and build python package
  3. Make a tag if tests pass
  4. Upload repository by calling pythonpackage agains.

Copy link
Member

Choose a reason for hiding this comment

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

Someone has to follow the release checklist and create the pyhelics release: GMLC-TDC/HELICS#2661

.github/workflows/pythonpackage.yml Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
set(HELICS_ZMQ_SUBPROJECT ON)
set(HELICS_ZMQ_FORCE_SUBPROJECT ON)
set(HELICS_ENABLE_ZMQ_CORE ON) # TODO: vendor libzmq similar to other 3rd party libraries
set(HELICS_DISABLE_BOOST OFF) # TODO: headers are just needed to compile, maybe tell users to have it installed as prereq for building from source?
Copy link
Member

Choose a reason for hiding this comment

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

I think we should users to have it installed as a pre-requisite for buidling from source. I think we need to update the pyproject.toml file to exclude the copy of boost that was downloaded (or have cibuildwheel on Linux stick it in a folder outside of the repository clone). A copy of Boost has made its way into the current source dist, making it take up about 250MB compressed -- or 1+ GB uncompressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've moved it to /tmp/boost. I'm still not sure why it got included in the sdist.

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