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

Add-Kedro viz-Main-Git #21441

Closed
wants to merge 33 commits into from
Closed

Conversation

rxm7706
Copy link
Contributor

@rxm7706 rxm7706 commented Dec 5, 2022

Goal - Create a conda package directly from the python src distribution
0 https://pypi.org/project/kedro-viz/#files

Current state - 8/12/2023

  • Only able to build by pointing to git repo directly (main)

Known Issues - to get to building off pypi packaged source

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged.
  • Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

@rxm7706
Copy link
Contributor Author

rxm7706 commented Dec 5, 2022

@conda-forge-admin, please rerender

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/kedro-viz) and found it was in an excellent condition.

@rxm7706
Copy link
Contributor Author

rxm7706 commented Dec 5, 2022

@conda-forge-admin, please rerender

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/kedro-viz) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipes/kedro-viz:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/kedro-viz) and found it was in an excellent condition.

@rxm7706
Copy link
Contributor Author

rxm7706 commented Dec 21, 2022

@conda-forge-admin, please rerender

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/kedro-viz) and found it was in an excellent condition.

@astrojuanlu
Copy link
Member

Hi @rxm7706! FYI I tried to give this a stab as well, cherry picking your branch and poking around #22142 still haven't had time to finish it, but at least reported a couple of issues upstream that you might want to look into: kedro-org/kedro-viz#1267 (hence sdists cannot be used) and kedro-org/kedro-viz#1278 (hence requiring NODE_OPTIONS=--openssl-legacy-provider).

add astrojuanlu and NODE_OPTIONS=--openssl-legacy-provider
@rxm7706
Copy link
Contributor Author

rxm7706 commented Apr 15, 2023

Hi @rxm7706! FYI I tried to give this a stab as well, cherry picking your branch and poking around #22142 still haven't had time to finish it, but at least reported a couple of issues upstream that you might want to look into: kedro-org/kedro-viz#1267 (hence sdists cannot be used) and kedro-org/kedro-viz#1278 (hence requiring NODE_OPTIONS=--openssl-legacy-provider).

Thanks @astrojuanlu ; taking a look - I've added your NODE_OPTIONS change, updated the pypi version and package.json, also added you to maintainers. This should at least prevent mixing issues related to the older version on pypi and outdated package.json

@rxm7706
Copy link
Contributor Author

rxm7706 commented Aug 11, 2023

@conda-forge-admin, please rerender

@rxm7706
Copy link
Contributor Author

rxm7706 commented Aug 11, 2023

@conda-forge-admin, please rerender

@astrojuanlu
Copy link
Member

😮 so the build process is working, finally?

@@ -0,0 +1,166 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

This would force us to keep this package.json and the one upstream in sync, can't we get it from the repository?

@rxm7706
Copy link
Contributor Author

rxm7706 commented Aug 11, 2023

Quite a bit still needs to be done upstream - since we need to have the build working off the packaged src from pypi.. (not github src) - yes, package.json will need to be included into the pypi src package (also the license files)

@rxm7706 rxm7706 changed the title Kedro viz add setuptools Kedro viz -Main Aug 12, 2023
@rxm7706 rxm7706 changed the title Kedro viz -Main Add-Kedro viz-Main-Git Aug 12, 2023
@rxm7706 rxm7706 closed this Aug 12, 2023
@rxm7706 rxm7706 deleted the Kedro-Viz-Add-setuptools branch August 12, 2023 09:49
@rxm7706 rxm7706 restored the Kedro-Viz-Add-setuptools branch August 12, 2023 09:50
@rxm7706 rxm7706 reopened this Aug 12, 2023
@rxm7706
Copy link
Contributor Author

rxm7706 commented Aug 12, 2023

@conda-forge-admin, please rerender

@rxm7706
Copy link
Contributor Author

rxm7706 commented Aug 12, 2023

@astrojuanlu I've summarized all issues I am aware of in the title. Please take a look, and let me know if you have any additional comments / suggestions on how to get this moving again

@astrojuanlu
Copy link
Member

how to get this moving again

I think we need to work upstream on https://github.com/kedro-org/kedro-viz/.

See kedro-org/kedro-viz#1267 (comment) for some ideas on how to solve the JS part (the difficult one). For the licenses, it should be as easy as modifying MANIFEST.in accordingly.

If we bundle the JS files with the sdist upstream, then the conda-forge recipe would not need Node and we could skip the Node 18 problem, if I'm not mistaken.

@rxm7706
Copy link
Contributor Author

rxm7706 commented Aug 16, 2023

closing this - as https://anaconda.org/conda-forge/kedro-viz is now available and the feedstock is working as expected.

To summarize

  1. The gap seemed to come down to differences between the targ.gz on pypi vs using the source tar ball form the kedro github repo..
  2. Also changing pip install to pick the package directory.
  3. This only fixes conda install, and some of the open upstream issues with pip install / making source tarball on pip and github release match - still exist.

@rxm7706 rxm7706 closed this Aug 16, 2023
@astrojuanlu
Copy link
Member

Oh wow, I was unaware of #23679, amazing! Didn't occur to me to use the tarball from GH. Still not 100 % sure why it works, but glad it does anyway. Thanks @rxm7706 for your effort!

@astrojuanlu
Copy link
Member

Sadly it was not as easy, issue incoming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants