kedro-viz icon indicating copy to clipboard operation
kedro-viz copied to clipboard

Fix packaging

Open astrojuanlu opened this issue 1 year ago • 5 comments

Description

Make sdists self-contained, hence fix #1267

Development notes

Commits:

  1. cf738296 Remove direct invokations of setup.py, which have been deprecated for about 2 years https://blog.ganssle.io/articles/2021/10/setup-py-deprecated.html
  2. 44538613 Migrate to pyproject.toml using @MehdiNV work in #1584 as baseline with some updates, hence fix #1527
  3. 495a54b1 Migrate from setuptools to hatchling https://hatch.pypa.io/, same backend vizro uses (cc @antonymilne), as a preparation to eventually address #1611 and also fix #1267
  4. e22a6ba0 Move README.md to package/ and create root symlink to retain GitHub preview

QA notes

  1. Run make package in main and with this PR, save the resulting sdist and wheel in dist.old and dist respectively
  2. Unzip the resulting .tar.gz, and compare the PKG-INFO files of both sdists - there should be only lines out of order and other non consequential changes directories with main and with this PR.
  3. Compare the tree structure of both sdists - only metadata files should have changed.
--- old.tree	2024-02-27 00:41:59
+++ new.tree	2024-02-27 00:42:29
@@ -1,5 +1,6 @@
-dist.old/kedro-viz-7.1.0/
+dist/kedro_viz-7.1.0/
 ├── PKG-INFO
+├── README.md
 ├── kedro_viz
 │   ├── __init__.py
 │   ├── api
@@ -81,10 +82,7 @@
 │       ├── __init__.py
 │       ├── layers.py
 │       └── modular_pipelines.py
-├── setup.cfg
-├── setup.py
-└── tests
-    ├── test_import.py
-    └── test_server.py
+├── pyproject.toml
+└── requirements.txt
 
-19 directories, 69 files
+18 directories, 68 files
  1. Unzip both wheel files and compare the *.dist-info directories - there should be only lines out of order but the number of files in RECORD should be basically the same.
...
diff --color=auto -u dist.old/kedro_viz-7.1.0.dist-info/RECORD dist/kedro_viz-7.1.0.dist-info/RECORD
--- dist.old/kedro_viz-7.1.0.dist-info/RECORD   2024-02-26 23:37:50
+++ dist/kedro_viz-7.1.0.dist-info/RECORD       2020-02-02 00:00:00
@@ -62,8 +62,7 @@
 kedro_viz/services/__init__.py,sha256=-ZfXYz5XecClJZK-cjZsoRrCpQCThwc3TXvCyDs0Sek,186
 kedro_viz/services/layers.py,sha256=_JO8mbmA0ZzAMN2dcXVaDX89Gr_QtJDWtmDnUeX49gc,5263
 kedro_viz/services/modular_pipelines.py,sha256=Tx3ekpwjt981jmljezxgWlOVaWdxgVADKCY1QyIUC4Q,4064
-kedro_viz-7.1.0.dist-info/METADATA,sha256=j_RijhLdEdn3vEgHlpCUDyCGHq6u4Ho8zkVJ9VT_xwE,11619
-kedro_viz-7.1.0.dist-info/WHEEL,sha256=oiQVh_5PnQM0E3gPdiz09WCNmwiHDMaGer_elqB3coM,92
+kedro_viz-7.1.0.dist-info/METADATA,sha256=tkqndzIrnu7hy0Pnj760FEOCApMYIBI9n0e2gxoo6vg,11759
+kedro_viz-7.1.0.dist-info/WHEEL,sha256=TJPnKdtrSue7xZ_AVGkp9YXcvDrobsjBds1du3Nx6dc,87
 kedro_viz-7.1.0.dist-info/entry_points.txt,sha256=yCRpAmWDqux5fspKHZ03tRjHkmIjs8ypb1m3XuvWNMI,228
-kedro_viz-7.1.0.dist-info/top_level.txt,sha256=gWY-UrKtMq-3SUTv5Zww2hc6_ldAln47aYqCihTOyew,10
 kedro_viz-7.1.0.dist-info/RECORD,,
  1. Verify that pip install ./dist.old/...tar.gz doesn't work (hence #1267) and pip install ./dist/...tar.gz fixes the issue
  2. Launch kedro viz run on a test project after installing the new sdist, verify that everything works as expected (the HTML files are included)

Checklist

  • [ ] Read the contributing guidelines
  • [ ] Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • [ ] Updated the documentation to reflect the code changes
  • [ ] Added new entries to the RELEASE.md file
  • [ ] Added tests to cover my changes

astrojuanlu avatar Feb 26 '24 23:02 astrojuanlu

package/kedro_viz/data_access/repositories/graph.py:46:8: R1737: Use 'yield from' directly instead of yielding each element one by one (use-yield-from)

linting failures unrelated

astrojuanlu avatar Feb 26 '24 23:02 astrojuanlu

The README.md hack didn't get me very far though, now the e2e tests are failing. Exhausted a couple of avenues with Hatch and gave up for the day, asked a upstream https://github.com/pypa/hatch/discussions/1286

astrojuanlu avatar Feb 27 '24 00:02 astrojuanlu

This is amazing!!!! -- thanks @astrojuanlu ! liking the nerd-snipe you.

rashidakanchwala avatar Feb 27 '24 09:02 rashidakanchwala

Amazing PR! This is a HUGE improvement.

Just two things I'm curious about:

  1. What is the motivation for "https://github.com/kedro-org/kedro-viz/commit/e22a6ba0df118d2941ec26770cc1f8d59704db68 Move README.md to package/ and create root symlink to retain GitHub preview"?
  2. What further steps would be needed to make pip install git+... on kedro-viz work?

antonymilne avatar Feb 28 '24 10:02 antonymilne

  1. setup.py had a open("../README.md"), that's not possible with static pyproject.toml (neither with setuptools nor with hatchling). I asked upstream https://github.com/pypa/hatch/discussions/1286 so far no response
  2. That's the topic of https://github.com/kedro-org/kedro-viz/issues/1611 and will require a custom hook that builds the assets upon installation, but that's for a separate PR 🙃

astrojuanlu avatar Feb 28 '24 11:02 astrojuanlu

Thanks @ravi-kumar-pilla ! I’ll double check the final state after you solved the conflicts

astrojuanlu avatar Jul 10 '24 07:07 astrojuanlu

This looks good to me 👍🏼 Beware of the moving README.md as it will conflict with #1745 @jitu5 fyi

ravi-kumar-pilla avatar Jul 10 '24 23:07 ravi-kumar-pilla