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

Poetry build #300

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Poetry build #300

wants to merge 9 commits into from

Conversation

jake-aft
Copy link
Contributor

@jake-aft jake-aft commented Dec 4, 2022

Current behaviour

pyproject.toml uses flint to build

New expected behaviour

Issue: Move setup.py functionality to poetry for pre-check and package build and release - #275

Build and publish the package with poetry in the pyproject.toml

I'd like to recommend that we put this on a branch to further test and validate it is doing everything we want/need it to do

Unsure of the Makefile that builds the docs and how it might be integrated into pyproject.toml with poetry
Need to review some of the pre-commit-config options

I am working on poetry build / publish instructions these will need to be integrated with the docs build instructions for whomever will be building the package not sure if this is something we need in the repo?

The pyproject.toml needs to have the version changed to the version that will be correct at the time of merge

Change logs

added exclude to pyproject.toml of the tests/rdf_tests to not be included in the package

Updated pre-commit-config to check the poetry additions
Updated pyproject.toml to use poetry build and dependency layouts

Size of the package is now ~50 K for both whl and zip

@Mec-iS
Copy link
Contributor

Mec-iS commented Dec 7, 2022

Is it possible to have merged all the three PRs about installing/building in this branch and close the other ones? Once they are in the same branch will be easier to review.

@jake-aft
Copy link
Contributor Author

@Mec-iS i think it is better to have the two separate.

The other pr #299 is really quite minor and mostly updated install instructions, so there is limited risk or impact. Adding Poetry and Pipenv to the install instructions.

This one is a significant change that uses poetry to build the kglab package for distribution. I do think this needs to be explored in some detail to make sure that we do want to move to a poetry build/dist model and the implications of moving to that from the existing build/dist tools.

As an example, I do not understand completely how the current documentation is generated, and how that may need to change or not if we move to poetry This type of thing is something that should be thought about by the team and others that have a more complete picture of the way the build is working today and be able to understand what things may be impacted by the changes.

@jake-aft
Copy link
Contributor Author

jake-aft commented Aug 8, 2023

@ceteri and @Mec-iS is the poetry build/publish still something you want to have fopr the product? I am going to have some time to revisit this if it is still wanted?

@ceteri
Copy link
Collaborator

ceteri commented Aug 8, 2023

Thank you kindly @jake-aft !
I've got time cleared out, and I'll work through the build. It's a little tricky given how the notebooks interact with generated docs, so I'll need to guide it a bit through this (most excellent) transition :)

@jake-aft
Copy link
Contributor Author

jake-aft commented Aug 8, 2023

@ceteri let me know if you want me to merge the two as @Mec-iS suggested. Also happy to help out on the docs if I would be of use?

ceteri
ceteri previously approved these changes Aug 8, 2023
Copy link
Collaborator

@ceteri ceteri left a comment

Choose a reason for hiding this comment

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

many thanks @jake-aft !!

Copy link
Collaborator

@ceteri ceteri left a comment

Choose a reason for hiding this comment

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

I ran into some problems with the poetry build where pytest unit tests could not be run. Also, GH got this PR wedged in a Merge Hell state where my only real option was to fork your repo into a repo on my personal account, then resolve merge conflicts from there.

So I'm refactoring to get some of these changes in, verify that they are stable, then pull in more.

See:

@ceteri ceteri dismissed their stale review August 9, 2023 00:20

found unit tests issues

@ceteri
Copy link
Collaborator

ceteri commented Aug 9, 2023

It's probably better to start from a branch based on the most recent main here, then back in the changes needed for a poetry build?

Otherwise there may not be a good way around the merge conflicts.

@jake-aft
Copy link
Contributor Author

jake-aft commented Aug 9, 2023

@ceteri I'll get the latest main, and resubmit

@jake-aft jake-aft marked this pull request as draft September 20, 2023 19:05
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.

3 participants