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

re-organize requirements files #1033

Open
wants to merge 6 commits into
base: master
from
Open

re-organize requirements files #1033

wants to merge 6 commits into from

Conversation

@bitnik
Copy link
Collaborator

bitnik commented Dec 18, 2019

I tried to re-organize requirements files and reduce them

  1. added instruction to install helm-chart/images/binderhub/requirements.txt when installing BinderHub locally with minikube. this is also what travis does and I think they should be same f we want to have same results from tests.

  2. removed pycurl from requirements.txt because it is already included in helm-chart/images/binderhub/requirements.txt. with this we don't have to manually update doc-requirements.txt each time requirements.txt is updated.

  3. removed jupyterhub from dev-requirements.txt because it is not required for building docs, it is already included in helm-chart/images/binderhub/requirements.txt and they (dev-requirements.txt and helm-chart/images/binderhub/requirements.txt) are installed together.

@@ -217,7 +217,7 @@ sudo apt install socat
1. Install BinderHub and its development requirements:

```bash
python3 -m pip install -e . -r dev-requirements.txt
python3 -m pip install -e . -r dev-requirements.txt -r helm-chart/images/binderhub/requirements.txt

This comment has been minimized.

Copy link
@manics

manics Jan 10, 2020

Member

It should be possible to nest requirements files, try adding

-r helm-chart/images/binderhub/requirements.txt

to the end of dev-requirements.txt?

This comment has been minimized.

Copy link
@bitnik

bitnik Jan 13, 2020

Author Collaborator

@manics I did it, but couldn't decide if I should update .travis.yaml (by removing line 22 and replacing line 19 with pip install --upgrade . -r dev-requirements.txt) or leave it as it is.

@bitnik bitnik force-pushed the gesiscss:reqs branch from d6dafe7 to 489012a Jan 13, 2020
@manics

This comment has been minimized.

Copy link
Member

manics commented Jan 13, 2020

Hmm, this might require some investigation:

binderhub/.travis.yml

Lines 19 to 22 in 0b5080a

- pip install --upgrade -r dev-requirements.txt
- npm install
- npm run webpack
- pip install --upgrade . -r helm-chart/images/binderhub/requirements.txt

The npm commands above are also in setup.py:

binderhub/setup.py

Lines 21 to 26 in 0b5080a

if os.name == "nt":
subprocess.check_call(['npm.cmd', 'install'])
subprocess.check_call(['npm.cmd', 'run', 'webpack'])
else:
subprocess.check_call(['npm', 'install'])
subprocess.check_call(['npm', 'run', 'webpack'])

You could try removing all four travis lines and see if pip install --upgrade -r dev-requirements.txt will automatically run the required npm commands?

If it doesn't then you could do what you suggested, remove line 19 and update 22? If npm is only generating Javascript/CSS assets it shouldn't matter that the python dependencies are missing.

# install BinderHub dependencies. We manually list them here because some
# dependencies (like pycurl) can't be installed on ReadTheDocs and aren't
# needed to build the docs.
kubernetes==9.0.*

This comment has been minimized.

Copy link
@betatim

betatim Jan 14, 2020

Member

How will these dependencies now get installed on read the docs?

This comment has been minimized.

Copy link
@bitnik

bitnik Jan 14, 2020

Author Collaborator

with -e ., setup.py installs requirements.txt

install_requires=requirements,

Actually now I am thinking we even don't need here -e/--editable flag, only . should be enough. what do you think?

@@ -10,4 +8,3 @@ prometheus_client
python-json-logger
jupyterhub
jsonschema
pycurl

This comment has been minimized.

Copy link
@betatim

betatim Jan 14, 2020

Member

I was expecting to see this line be replaced with an include/reference to another requirements file to make sure pycurl continues to be installed. What did I miss?

This comment has been minimized.

Copy link
@bitnik

bitnik Jan 14, 2020

Author Collaborator

right, I would expect that too and now I am confused.

pycurl is installed in helm-chart/images/binderhub/requirements.txt


And when I go though the docs, "User interface changes" is the only place that we only install requirements.txt and there we don't need to have pycurl. For example, when we run tests locally, we have to install dev-requirements.txt which installs also helm-chart/images/binderhub/requirements.txt.

@bitnik

This comment has been minimized.

Copy link
Collaborator Author

bitnik commented Jan 14, 2020

You could try removing all four travis lines and see if pip install --upgrade -r dev-requirements.txt will automatically run the required npm commands?

No it doesn't run npm commands in that case. It does when -e/--editable flag is added.

@bitnik

This comment has been minimized.

Copy link
Collaborator Author

bitnik commented Jan 14, 2020

after learning that pip install -e . also runs npm commands in setup.py, I think npm instructions in User interface changes section in docs is redundant. But I think it is nice to have it explicitly, so people now how these js and css files are generated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.