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

Update pylint in the test worker image not compatible with f style strings #560

Closed
jlewi opened this issue Jan 8, 2020 · 4 comments
Closed

Comments

@jlewi
Copy link
Contributor

jlewi commented Jan 8, 2020

I get different behavior when running pylint in the test worker image then when running it locally.

Example PR
https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/kubeflow_testing/559/kubeflow-testing-presubmit/1214777447178833920/

When run in the worker image I get

util.py                     72 INFO     ************* Module kubeflow.testing.cd.update_kf_apps
util.py                     72 INFO     E: 45, 0: invalid syntax (<string>, line 45) (syntax-error)
util.py                     46 INFO     Running: pylint --rcfile=/mnt/test-data-volume/kubeflow-testing-presubmit-py-unittests-559-d51aded-3920-83d3/src/kubeflow/testing/.pylintrc /mnt/test-data-volume/kubeflow-testing-presubmit-py-unittests-559-d51ade
d-3920-83d3/src/kubeflow/testing/py/kubeflow/testing/cd/update_kf_apps_test.py

I do not get that error when running locally.

Locally I'm using version

pylint3 --version
pylint3 2.2.2
astroid 2.1.0
Python 3.7.5rc1 (default, Oct  2 2019, 04:19:31) 
[GCC 8.3.0]

in gcr.io/kubeflow-ci/test-worker@sha256:fbfb9141c02e9a50e3679f84637fc2d7c142fe9eaa6b9f55a3c9f8a710000837

pylint --version
No config file found, using default configuration
pylint 1.8.2, 
astroid 1.6.1
Python 3.5.2 (default, Oct  8 2019, 13:06:37) 
[GCC 5.4.0 20160609]

I tried installing pylint3 in the container and got the same error

pylint3 --version
Using config file /git_kubeflow-testing/.pylintrc
pylint3 1.8.2, 
astroid 1.6.1
Python 3.5.2 (default, Oct  8 2019, 13:06:37) 
[GCC 5.4.0 20160609]

It looks like the syntax error is caused by f-style strings.

e.g.

  raise ValueError(f"Repository {r['name']} is missing param url") # pylint: disable=syntax-error

I suspect to fix this we may need to upgrade the python version to 3.7

jlewi pushed a commit to jlewi/testing that referenced this issue Jan 8, 2020
jlewi pushed a commit to jlewi/testing that referenced this issue Jan 9, 2020
… applications

* Define a config file to list
  i) Relevant information about each application to build
     e.g the location of its source, the location of its kustomize manifest
 ii) The versions (i.e. branches) of the various repositories to build from

* Create a python script that takes the cross product of applications and
  versions and creates a tekton PipelineRun to build the docker image
  and update the manifest.

* Fix a bug in update_manifests with image_name not being set.
Related to kubeflow#450: Continuous delivery of Kubeflow applications

Here are some autogenerated PRs updating the manifests

  * kubeflow/manifests#697
  * kubeflow/manifests#696

* Fix email of kubeflow bot to pass CLA check.

  * Fix kubeflow#557

Skip lint due to kubeflow#560
k8s-ci-robot pushed a commit that referenced this issue Jan 9, 2020
… applications (#559)

* Define a config file to list
  i) Relevant information about each application to build
     e.g the location of its source, the location of its kustomize manifest
 ii) The versions (i.e. branches) of the various repositories to build from

* Create a python script that takes the cross product of applications and
  versions and creates a tekton PipelineRun to build the docker image
  and update the manifest.

* Fix a bug in update_manifests with image_name not being set.
Related to #450: Continuous delivery of Kubeflow applications

Here are some autogenerated PRs updating the manifests

  * kubeflow/manifests#697
  * kubeflow/manifests#696

* Fix email of kubeflow bot to pass CLA check.

  * Fix #557

Skip lint due to #560
@jlewi
Copy link
Contributor Author

jlewi commented Jan 16, 2020

I think we can do something like the following to install the later version of python3

RUN apt-get update -y && \
    apt-get install -y python3.8 python3-pip && \
    ln -sf /usr/bin/python3.8 /usr/bin/python

RUN python3.8 -m pip install fire watchdog

I think we want to use python3.8 -m pip to install any pip packages as that ensures they are installed in the dist packages location corresponding to python3.8

@jlewi
Copy link
Contributor Author

jlewi commented Jan 22, 2020

Example Dockerfile
https://github.com/kubeflow/testing/blob/master/apps-cd/Dockerfile

I tried updating our existing worker images to ubuntu:18.04 and some of the existing steps failed.

jlewi pushed a commit to jlewi/testing that referenced this issue Jan 22, 2020
…6.04)

  * We need to install python3.8 and it looks like it wasn't available in
    apt for 16.04

  * related to: kubeflow#560
jlewi pushed a commit to jlewi/testing that referenced this issue Feb 11, 2020
* Related to kubeflow#560
* We need this because we need a newer (3.8) version of pylint
  to allow for python3.8 syntax like f strings.

* In this pass we eschewed the following packages which I don't think we
  need anymore

  * ksonnet
  * helm
  * glide (should be using go modules and staged builds)
  * docker
  * nodejs (should be using staged builds)

* Create a skaffold config to build the image.
jlewi pushed a commit to jlewi/examples that referenced this issue Feb 11, 2020
* Lint is failing because we are still runing python2 for lint

* kubeflow/testing#560 is related to building an updated image with python3.8
  compatible version of lint so we can support f style strings.

* However, the unittests for kubeflow examples are still written in
  ksonnet. Its not worth trying to update that so we just
  remove that test for now. The test was just running lint

* We should really see about using Tekton to write the workflows

  see kubeflow/testing#425
k8s-ci-robot pushed a commit to kubeflow/examples that referenced this issue Feb 12, 2020
* Lint is failing because we are still runing python2 for lint

* kubeflow/testing#560 is related to building an updated image with python3.8
  compatible version of lint so we can support f style strings.

* However, the unittests for kubeflow examples are still written in
  ksonnet. Its not worth trying to update that so we just
  remove that test for now. The test was just running lint

* We should really see about using Tekton to write the workflows

  see kubeflow/testing#425
k8s-ci-robot pushed a commit that referenced this issue Feb 12, 2020
* Create a python3.8 version of the worker image

* Related to #560
* We need this because we need a newer (3.8) version of pylint
  to allow for python3.8 syntax like f strings.

* In this pass we eschewed the following packages which I don't think we
  need anymore

  * ksonnet
  * helm
  * glide (should be using go modules and staged builds)
  * docker
  * nodejs (should be using staged builds)

* Create a skaffold config to build the image.

* Add gcloud and kube-rsa.
@stale
Copy link

stale bot commented Apr 23, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in one week if no further activity occurs. Thank you for your contributions.

@stale
Copy link

stale bot commented Apr 30, 2020

This issue has been closed due to inactivity.

@stale stale bot closed this as completed Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant