Skip to content

Conversation

@jimexist
Copy link
Member

@jimexist jimexist commented Dec 27, 2017

this is really a trivial change and so far I haven't been able to find time to adjust the docker file accordingly.

This change is Reviewable

@k8s-ci-robot
Copy link

Hi @jimexist. Thanks for your PR.

I'm waiting for a kubernetes or tensorflow 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.

@coveralls
Copy link

coveralls commented Dec 27, 2017

Coverage Status

Coverage increased (+0.1%) to 37.915% when pulling d4d5c04 on Jimexist:use-pipenv into a704246 on tensorflow:master.

Copy link
Contributor

@jlewi jlewi left a comment

Choose a reason for hiding this comment

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

Thanks. I'm not familiar with pipenv so a couple questions

  • How does this relate to requirements.txt? Can we get rid of requirements.txt?

  • How will we use pipenv? Will pipenv be used to produce a pip package that will be used to install the py package in our containers?

@jlewi
Copy link
Contributor

jlewi commented Dec 28, 2017

/ok-to-test

@jimexist
Copy link
Member Author

How does this relate to requirements.txt? Can we get rid of requirements.txt?

yes. and you can always use pipenv to re-generate one

How will we use pipenv? Will pipenv be used to produce a pip package that will be used to install the py package in our containers?

for docker installation I can think of a template docker file like this (hope this can explain better):

FROM python:3.6

COPY Pipfile Pipfile.lock /my/app/path/

WORKDIR /my/app/path/

# see https://github.com/pypa/pipenv/blob/master/docs/advanced.rst#-deploying-system-dependencies
RUN pip3 install pipenv && pipenv install --system --deploy

COPY ./python_code /my/app/path/

CMD ["python", "script.py"]

@jimexist jimexist changed the title feat(pipenv): Use pipenv to lockdown python dependencies feat(pipenv): Use pipenv to lock down python dependencies Dec 29, 2017
@jimexist
Copy link
Member Author

@jlewi now that you mentioned this - I found out that there are actually two use cases for python, one for test-infra (the airflow docker image) and one for releases (the /py/ folder).

the use case for pipenv could be... well, managing two separately. I can thinking this might sound like an overkill, but I'm kind of used to pipenv and hence the PR. Feel free to close it if you think it is too much. Personally I think it is a seamlessly working tool.

@jlewi
Copy link
Contributor

jlewi commented Dec 29, 2017

According to this pipenv is the officially recommended package manager so I think it makes sense to use it.

Should we delete requirements.txt and update the developer_guide to refer to pipenv?

@coveralls
Copy link

coveralls commented Dec 31, 2017

Coverage Status

Coverage increased (+0.08%) to 28.601% when pulling 34df25c on Jimexist:use-pipenv into ed7cae7 on tensorflow:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 28.518% when pulling 34df25c on Jimexist:use-pipenv into ed7cae7 on tensorflow:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 28.518% when pulling 34df25c on Jimexist:use-pipenv into ed7cae7 on tensorflow:master.

@jimexist
Copy link
Member Author

jimexist commented Jan 1, 2018

@jlewi sure, WIP.

@jimexist
Copy link
Member Author

jimexist commented Jan 2, 2018

@jlewi developer guide updated, PTAL

@coveralls
Copy link

coveralls commented Jan 2, 2018

Coverage Status

Coverage decreased (-0.1%) to 28.416% when pulling 0b8f20f on Jimexist:use-pipenv into ed7cae7 on tensorflow:master.

@jlewi
Copy link
Contributor

jlewi commented Jan 2, 2018

LGTM but looks like the build is still broken because of lint issues.

@jlewi
Copy link
Contributor

jlewi commented Jan 3, 2018

@jimexist build should be fixed by #257 so I think you just need to merge in the most recent changes and push.

@coveralls
Copy link

coveralls commented Jan 3, 2018

Coverage Status

Coverage remained the same at 28.458% when pulling 159623f on Jimexist:use-pipenv into 2793089 on tensorflow:master.

@jimexist
Copy link
Member Author

jimexist commented Jan 5, 2018

@jlewi for some reason the build failed

@jlewi
Copy link
Contributor

jlewi commented Jan 5, 2018

/test-all

@jlewi
Copy link
Contributor

jlewi commented Jan 5, 2018

/test all

@jlewi jlewi merged commit 190394d into kubeflow:master Jan 5, 2018
sutaakar pushed a commit to sutaakar/training-operator that referenced this pull request Mar 25, 2025
Signed-off-by: konflux-internal-p02 <170854209+konflux-internal-p02[bot]@users.noreply.github.com>
Co-authored-by: konflux-internal-p02[bot] <170854209+konflux-internal-p02[bot]@users.noreply.github.com>
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