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

build: install jinja2 from binary #443

Merged
merged 3 commits into from
Apr 24, 2024

Conversation

DnPlas
Copy link
Contributor

@DnPlas DnPlas commented Apr 23, 2024

This commit installs jinja2 (an install dependency of charmed-kubeflow-chisme) as a binary to avoid running into the build time issues described in canonical/bundle-kubeflow#883.

Part of canonical/bundle-kubeflow#883

Test instructions

  • Test that the failure is present
  1. Clone this repository and cd into a charm cd charms/kfp-api
  2. Run charmcraft clean to make sure the pack will be done from scratch
  3. Run charmcraft pack -v (from main) and ensure your get the following error during the pip install process, it should happen when collecting jinja2:
::    :: Collecting jinja2==3.1.2                                                                                                                                                            
::    ::   Using cached Jinja2-3.1.2.tar.gz (268 kB)                                                                                                                                         
::    ::     ERROR: Command errored out with exit status 1:  
...
::    ::         from .environment import Environment as Environment                                                                                                                         
::    ::       File "/tmp/pip-install-6xydwp2_/jinja2/src/jinja2/environment.py", line 14, in <module>                                                                                       
::    ::         from markupsafe import Markup                                                                                                                                               
::    ::     ModuleNotFoundError: No module named 'markupsafe'                                                                                                                               
::    ::     ----------------------------------------   

Please NOTE the charm will build, the issue with the CI is that it will stop the execution on error because charms are built from a test case.

  • Test the change in this PR
  1. Checkout to the branch in this PR
  2. Execute the same steps as before, make sure to run charmcraft clean
  3. The charm should build w/o any errors
  • Test that charms work
  1. On a clean environment that simulates the CI, run integration tests tox -e bundle-integration-v1 -- --model kubeflow --bundle=./tests/integration/bundles/kfp_latest_edge.yaml.j2
  2. All charms except for kfp-metadata-writer/0 should go to active. It has to show this msg in the status: [relation:grpc] Expected data from exactly 1 related applications - got 0.. This will be fixed in refactor: use k8s_service_info lib instead of SDI #436.
  3. There could be missing relations as the step in (1) will exit before continuing with adding all relations because of (2). I have only noticed three missing relations juju relate istio-pilot istio-ingressgateway; juju relate envoy mlmd; juju relate kfp-profile-controller minio, but please add any other that could be missing if the charm complains.
  4. mlmd has to be trusted from now on, that has to be done manually for this test juju trust mlmd --scope=cluster. This is also fixed in refactor: use k8s_service_info lib instead of SDI #436.

Expected state:

Unit                        Workload  Agent  Address     Ports          Message       
argo-controller/0*          active    idle   10.1.91.16                                                                                              
envoy/0*                    active    idle   10.1.91.17                                                                                        
istio-ingressgateway/0*     active    idle   10.1.91.19                                                                                                                   
istio-pilot/0*              active    idle   10.1.91.20                                                                                                             
kfp-api/0*                  active    idle   10.1.91.18                                                                                       
kfp-db/0*                   active    idle   10.1.91.25                 Primary                                                                                    
kfp-metadata-writer/0*      blocked   idle   10.1.91.21                 [relation:grpc] Expected data from exactly 1 related applications - got 0.       
kfp-persistence/0*          active    idle   10.1.91.22                                                                                                   
kfp-profile-controller/0*   active    idle   10.1.91.24                                                                                                         
kfp-schedwf/0*              active    idle   10.1.91.26                                                                                                             
kfp-ui/0*                   active    idle   10.1.91.28                                                                                     
kfp-viewer/0*               active    idle   10.1.91.29                                                                                                      
kfp-viz/0*                  active    idle   10.1.91.30                                                                               
kubeflow-profiles/0*        active    idle   10.1.91.35                                                                                      
kubeflow-roles/0*           active    idle   10.1.91.31                                                                             
metacontroller-operator/0*  active    idle   10.1.91.32          
minio/0*                    active    idle   10.1.91.38  9000-9001/TCP                                                   
mlmd/0*                     active    idle   10.1.91.34  

This commit removes charmed-kubeflow-chisme from the requirements file to prevent
it to be built at charm build time, as some of its build deps are causing the
issue described in canonical/bundle-kubeflow#883.
Also in this commit the requirements-unit.in has been changed to include
charmed-kubeflow-chisme to install it during unit tests as it was
removed from the requirements.txt; therefore from the
requirements-unit.txt file.
Finally, this commit removes the "usage" legend from
requirements-unit.in as it is not a team standard anymore.

Part of canonical/bundle-kubeflow#883
@DnPlas
Copy link
Contributor Author

DnPlas commented Apr 23, 2024

The expected issue in the CI should only be ERROR cannot deploy bundle: cannot add relation between "kfp-metadata-writer:grpc" and "mlmd:grpc": no relations found

Copy link
Contributor

@ca-scribner ca-scribner left a comment

Choose a reason for hiding this comment

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

I have concerns about this. I'm not sure it will break anything, but I'm not sure it wont either and I'm not sure how to confirm it really is ok

Does chisme need to be removed from requirements.txt? My understanding was that charm-binary-python-packages was for listing packages you want to download binaries for, but then the requirements.txt file was still the file the defined all charm dependencies. Any overlap between these files would have no effect.

I'm not clear how charm-binary-python-packages - do you know whether it installs only the binary for the packages listed (equivalent to pip install just-chisme's-binary, or if it does a full pip install whatever-is-listed which installs the dependencies)? I'm worried that if chisme is in charm-binary-python-packages and removed from requirements.txt then dependencies of chisme wont be installed.

Chisme feels like an odd thing to add to charm-binary-python-packages since chisme doesn't have any compiled code. Would it make sense to put whatever binary chisme depends on and we're rebuilding into charm-binary-python-packages instead?

@DnPlas
Copy link
Contributor Author

DnPlas commented Apr 24, 2024

I have concerns about this. I'm not sure it will break anything, but I'm not sure it wont either and I'm not sure how to confirm it really is ok

I have added very detailed testing instructions. Please let me know if that helps.

Does chisme need to be removed from requirements.txt? My understanding was that charm-binary-python-packages was for listing packages you want to download binaries for, but then the requirements.txt file was still the file the defined all charm dependencies. Any overlap between these files would have no effect.

I don't think I follow this concern, sorry - In this PR I am removing chisme from requirements.txt and only leaving it in the charmcraft.yaml file.

EDIT: I was removing it, but I still don't fully understand the concern. If your concern is that anything listed in both requirements.txt and in charmcaft.yaml will have no effect on the install, well, AFAICT, whatever gets installed as a binary won't be re-installed from source even if it's listed in requirements.txt. You can verify that by looking at the logs of this PR, where jinja2 is listed in both reqs and charmcraft, but only installed once and not built from source as other charm deps.

I'm not clear how charm-binary-python-packages - do you know whether it installs only the binary for the packages listed (equivalent to pip install just-chisme's-binary, or if it does a full pip install whatever-is-listed which installs the dependencies)? I'm worried that if chisme is in charm-binary-python-packages and removed from requirements.txt then dependencies of chisme wont be installed.

This is an example of the command that runs at charm build time:

:: Running external command ['/root/parts/charm/build/staging-venv/bin/pip', 'install', '--no-binary=anyio,attrs,certifi,charset-normalizer,exceptiongroup,h11,httpcore,httpx,idna,importlib-resources,jsonschema,lightkube,lightkube-models,oci-image,ops,pip,pkgutil-resolve-name,pyrsistent,pyyaml,requests,serialized-data-interface,setuptools,sniffio,tenacity,urllib3,websocket-client,wheel,zipp', '--requirement=requirements.txt', 'charmed-kubeflow-chisme', 'pip', 'setuptools', 'wheel']                                                                                    

and this is the last log we get from that command:

::    :: Successfully installed MarkupSafe-2.1.5 anyio-3.7.1 attrs-23.1.0 certifi-2023.7.22 charmed-kubeflow-chisme-0.3.0 charset-normalizer-3.2.0 deepdiff-6.2.1 exceptiongroup-1.1.2 h11-0.14.0 httpcore-0.17.3 httpx-0.24.1 idna-3.4 importlib-resources-6.0.1 jinja2-3.1.3 jsonschema-4.17.3 lightkube-0.14.0 lightkube-models-1.27.1.4 oci-image-1.0.0 ops-2.5.0 ordered-set-4.1.0 pkgutil-resolve-name-1.3.10 pyrsistent-0.19.3 pyyaml-6.0.1 requests-2.31.0 ruamel.yaml-0.18.6 ruamel.yaml.clib-0.2.8 serialized-data-interface-0.7.0 sniffio-1.3.0 tenacity-8.2.2 urllib3-2.0.4 websocket-client-1.6.1 wheel-0.43.0 zipp-3.16.2                                                                                                                                                

As you can see, even if I removed jinja2 and MarkupSafe (as a consequence of removing chisme) from requirements.txt, they still get installed. These are defined as install dependencies of chisme.

Chisme feels like an odd thing to add to charm-binary-python-packages since chisme doesn't have any compiled code. Would it make sense to put whatever binary chisme depends on and we're rebuilding into charm-binary-python-packages instead?

When I was testing this yesterday it did not work for me, but I re-tried and it seems to be working just fine, if it's better I will just put jinja2 in charm-binary-python-pakcage.

@DnPlas DnPlas changed the title build: install charmed-kubeflow-chisme from binary build: install jinja2 from binary Apr 24, 2024
@@ -11,3 +11,7 @@ bases:
parts:
charm:
charm-python-packages: [setuptools, pip] # Fixes install of some packages
# Install jinja2 (a dependency of charmed-kubeflow-chisme) from binary to avoid build-time issues
# See https://github.com/canonical/bundle-kubeflow/issues/883
# Remove when https://github.com/canonical/charmcraft/issues/1664 is fixed
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this comment still apply? I think we only wait on 883 to remove this (1664 was a reason why this might be flaky or broken - when 1664 resolves we can't just remove jija2 from the binary list)

Copy link
Contributor

Choose a reason for hiding this comment

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

(this comment applies to all charms, but no need to make it that many times)

# Install jinja2 (a dependency of charmed-kubeflow-chisme) from binary to avoid build-time issues
# See https://github.com/canonical/bundle-kubeflow/issues/883
# Remove when https://github.com/canonical/charmcraft/issues/1664 is fixed
charm-binary-python-packages: [jinja2]
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this should be left unpinned, or if it should be pinned to the same thing as in requirements.txt.

Did some local testing and I saw we currently build the charm by downloading the binary for jinja=3.1.2, which is not the latest but is the version specified in requirements.txt. So I think somehow charmcraft is resolving this unpinned version by using the requirements.txt?

I'm asking more about it here, but probably leaving it unpinned here for now is ok

Copy link
Contributor

@ca-scribner ca-scribner left a comment

Choose a reason for hiding this comment

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

code lgtm. imo the comment about issue 1664 should be removed (see comment above) but its not a blocker. If I'm wrong, leave it in

@DnPlas DnPlas merged commit 79da85b into dnplas-dev-branch-fix-ci Apr 24, 2024
44 of 46 checks passed
DnPlas added a commit that referenced this pull request Apr 25, 2024
This PR merges the following changes in main as they were introduced in a development branch.

* ci: remove destructive mode from integration tests #441
* build: install jinja2 from binary #443
* refactor: use k8s_service_info lib instead of SDI #436
NohaIhab pushed a commit that referenced this pull request May 9, 2024
switch to jlumbroso/free-disk-space for freeing runner space (#428)

The previous space freeing method (easimon/maximize-build-space) at some point circa 2024-01 stopped freeing as much space, likely due to changes in the runner (~29GB free after it freed space).  Not sure why this happened, but jlumbroso/free-disk-space at time of this commit would get us up to ~45GB free on the runner without negative effects so we've switched to that.

Support functionality to override default images for kfp-profile-controller (#416)

* Support functionality to override default images for kfp-profile-controller

ci: remove destructive mode from integration tests (#441)

The charmcraft issues that forced us to use destructive mode are now fixed.

build: install `jinja2` from binary (#443)

This commit installs jinja2 (an install dependency of charmed-kubeflow-chisme) as a binary
to avoid running into the build time issues described in canonical/bundle-kubeflow#883.

Part of canonical/bundle-kubeflow#883

refactor: use k8s_service_info lib instead of SDI (#436)

* refactor: use k8s_service_info lib instead of SDI

Use the k8s_service_info for receiving the MLMD GRPC Service info instead of using
the SDI, as it will stop being supported soon.
This commit also ensures that mlmd runs with trust=True in the
integration tests.

Fixes #413

fix: Pin integration test dependencies in main (#434)

* pin integration test dependencies

Co-authored-by: Daniela Plascencia <[email protected]>

feat: Integrate ROCK in metadata-writer charm (#439)

chore: Bump o11y libs and remove obsolete juju topology (#446)

Ref canonical/bundle-kubeflow#880
Ref canonical/bundle-kubeflow#849

ci: bump juju to 3.5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants