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

py: Add requirements.txt #180

Merged
merged 1 commit into from
Nov 29, 2017
Merged

py: Add requirements.txt #180

merged 1 commit into from
Nov 29, 2017

Conversation

gaocegege
Copy link
Member

The requirements.txt works well for me, I think it is better to add it into the repo.

WDYT

Signed-off-by: Ce Gao ce.gao@outlook.com

@k8s-ci-robot
Copy link

Hi @gaocegege. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@DjangoPeng
Copy link
Member

Good job! I created an issue for 3rd party dependencies just now. @gaocegege Are you sure the requirement.txt including all of Python dependencies? What kind of Python environment are you developing and testing? Anaconda, Virtualenv?

@DjangoPeng
Copy link
Member

Ref: #181

@@ -0,0 +1,10 @@
Jinja2==2.9.5
Copy link
Member

Choose a reason for hiding this comment

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

Replace == with >= would be better. We don't need to specify an exact version of dependencies. Same as below.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK I will update it, but I am not sure if all the denpendencies are upwardly compatible.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, so we have to guarantee each items.

@gaocegege
Copy link
Member Author

@DjangoPeng

Glad to see you in GitHub :) I used virtualenv to setup a clear python environment, then I used pipreqs to generate the requirements.txt for py/. But it does not work well, then I added some missed dependencies into the text file mannually.

FYI, the version of python in the virtualenv is 3.5.2

@DjangoPeng
Copy link
Member

DjangoPeng commented Nov 28, 2017

@gaocegege Got you. Have you tested the Python 2.7.x? Because I'm using Anaconda including many pre-installed libs, my env is not clear. Would you mind testing the Python 2.7.x env?

@gaocegege
Copy link
Member Author

gaocegege commented Nov 28, 2017

re @DjangoPeng I tried with python 2.7.12 and it works well.

@DjangoPeng
Copy link
Member

Sounds good. Wait for your updating.

@jlewi Could you have a look?

@gaocegege
Copy link
Member Author

There is something when you run the release script, it fails to finish the push step: https://pastebin.com/R2PSMXpy

But I think it does not matter if you do not want to push the image.

Signed-off-by: Ce Gao <ce.gao@outlook.com>
@jlewi
Copy link
Contributor

jlewi commented Nov 28, 2017

Thank you.

@gaocegege Do you have gcloud installed? I think the error you are getting might be because you don't have gcloud installed. It looks like you changed the registry to use a docker hub docker/tf_operator:v20171128-8d6fa29
but the script is still trying to use gcloud to push it. So the script probably needs to be updated to not use gcloud for those registries.

@jlewi
Copy link
Contributor

jlewi commented Nov 28, 2017

/ok-to-test

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.

4 participants