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

Support TFX 1.14 #297

Merged
merged 6 commits into from
Feb 2, 2024
Merged

Support TFX 1.14 #297

merged 6 commits into from
Feb 2, 2024

Conversation

adriangay
Copy link
Collaborator

@adriangay adriangay commented Nov 2, 2023

This PR updates the KFP compiler build and test to support TFX 1.14. TFX 1.14 official container is built with Python 3.10, so compiler build/test is updated for that. It also removes support for Python 3.7, which is no longer required.

The following changes were required:

  • dependencies updated
    • python = ">=3.8,<3.11" - removes support for 3.7; adds support for 3.10
    • PyYAML = ">=5.4.1" - this was updated in conjunction with a breaking change (see below)
    • pytest = "7.4.0" - this was found to be necessary to get some pipeline unit tests to work, so also upgraded here
    • shapely = "<2" - see this Google AI Platform issue
  • updates to compiler Dockerfile to remove Python 3.7 and add 3.10
  • fixed breaking change in compiler unit test - yaml.load() now needs a Loader arg, or use safe_load() method. The latter was used.
  • updated integration-test Python versions
  • fixed integration-test - was missing --execution_mode arg and compiler expects --pipeline_config arg to be a file
  • in integration-test Dockerfile, the initial shapely downgrade on single pip command RUN pip install tfx==$TFX_VERSION "shapely<2" did not work and introduced null errors and segmentation faults when importing TFX packages in compiler.py. Following the exact instructions in the issue above, ie. pip install -U google-cloud-aiplatform "shapely<2" fixed the issue
  • tested v1 and v2 args on --execution_mode
  • while debugging the shapely issue, noticed ::// in PYTHONPATH in compile.sh- it obviously did 'work' before, but corrected it anyway
  • corrected compiler path in README.md

✅ unit tests passing

✅ acceptance test passing

✅ integration test passing

✅ prBuild passing

Note: the issue with Python version during compile build/test was due to Poetry installation picking up Python 3.11.2. It does not 'pick' the 'currently active' version in a multi-Pyhon installation. There is no easy way for force it, but you can tell it which to use. The line:

poetry env use /home/jenkins/.asdf/installs/python/3.10.12/bin/python

in the compiler makefile forces Poetry to use a specified available version, in this case 3.10.12. the Python bin path in Jenkins is not the same as local machines, which is not ideal.

@sky-uk sky-uk deleted a comment from adriangay Dec 20, 2023
@adriangay adriangay marked this pull request as ready for review December 21, 2023 19:30
argo/kfp-compiler/Makefile Outdated Show resolved Hide resolved
argo/kfp-compiler/Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@paoloambrosio-skyuk paoloambrosio-skyuk left a comment

Choose a reason for hiding this comment

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

My main concern is how to document the shapely pinning to version <2. Should it be an issue on this repo that we link from both places with a "FIXME" comment?

argo/kfp-compiler/poetry.lock Show resolved Hide resolved
@adriangay
Copy link
Collaborator Author

My main concern is how to document the shapely pinning to version <2. Should it be an issue on this repo that we link from both places with a "FIXME" comment?

It should be noted somehow. At least referring to the issue it fixes would explain to OSS users why it's there.

.metals/metals.mv.db Outdated Show resolved Hide resolved
@sky-uk sky-uk deleted a comment from adriangay Jan 2, 2024
@sky-uk sky-uk deleted a comment from adriangay Jan 2, 2024
Copy link
Contributor

@paoloambrosio-skyuk paoloambrosio-skyuk left a comment

Choose a reason for hiding this comment

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

Some more thoughts on versions:

  • .tool-versions
    • ✅ We should always use the latest Python version from pyproject.toml
    • ❎ We should always use the latest Poetry
  • argo/kfp-compiler/pyproject.toml
    • ✅ we should pin tfx to the latest supported version
    • ✅ Python version range should be set accordingly
  • argo/kfp-compiler/Makefile and argo/kfp-compiler/Dockerfile
    • ❎ We should build and test with all versions. In the PR 3.9 and 3.10 seem completely arbitrary, and 3.8 is missed.

.tool-versions Outdated Show resolved Hide resolved
argo/kfp-compiler/poetry.lock Show resolved Hide resolved
@paoloambrosio-skyuk paoloambrosio-skyuk changed the title Support tfx 1.14 Support TFX 1.14 Jan 9, 2024
@jmendesky jmendesky force-pushed the support-tfx-1.14 branch 3 times, most recently from 17bc044 to 7b62378 Compare January 9, 2024 16:10
@sky-uk sky-uk deleted a comment from adriangay Feb 2, 2024
@jmendesky jmendesky merged commit a336814 into master Feb 2, 2024
1 check passed
@jmendesky jmendesky deleted the support-tfx-1.14 branch February 2, 2024 14:41
@jmendesky jmendesky mentioned this pull request Feb 5, 2024
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