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

added Dockerfile #235

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

added Dockerfile #235

wants to merge 5 commits into from

Conversation

zenkavi
Copy link

@zenkavi zenkavi commented Jul 25, 2023

Not the most elegant way of doing it but so far I haven't run into any other issues with the image

Copy link
Collaborator

@AlexanderFengler AlexanderFengler left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request.
Just left a small note.

USER $NB_UID

## TODO: Move these to a requirements.txt and install in a virtual env instead of as root
RUN pip install --upgrade pip && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you try a minimal version here?
After installing the jupyterlab related stuff, pip install hssm (in the best case alone, but potentially with very few extra installs), should suffice.
I looks like you are manually installing a ton of dependencies that would be installed upon hssm install.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Admittedly I am no docker-expert, but this is what it looks like :))

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense. I'm not an expert either but I cleaned it up and made a new image and it seemed to work fine. I actually found another bug so it was good to review it again.

Before I send an updated PR though I had another question: how are the tests in the repo intended to be run? I used them to quickly check if the new image was working as expected. Do you use these internally ad hoc? Are they automated somehow with each build?

The only testing framework I've used in Python is unittest (https://docs.python.org/3/library/unittest.html#organizing-test-code). I can translate your tests and run them all when building an image. So if the tests are exhaustive then this would make sure that there wouldn't be an image with a broken version of hssm. The con for this is that each script in the tests directory would change slightly. But if you accept the format and adhere to it with future tests then they can always be run automatically with the same single command (python -m unittest).

Alternatively tests can stay as they are and I can in effect loop through everything in the directory. So if you have another pipeline these are already seamlessly going through that would not be messed up. I don't have a preference either way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you :).

As per Paul's answer below, I don't think you have to worry about running tests in the docker image. As long as you install from pip that is automatically guaranteed (but in fact since we run tests on every PR, the same is true if you install from the main branch on git, even though I would recommend pip).

@digicosmos86
Copy link
Collaborator

Thank you for doing this! I especially like that you also download the notebooks. We have recently changed the locations of these notebooks as of v0.1.3. If it is OK with you, I'd like to do a PR on your PR, so I can fix the URLs. I can also update how dependencies are installed and add a few CI workflows to build the images automatically.

To answer your question, we use pytest for testing. Tests are run for every push and every PR. They are also run at each release, before an update is pushed to PyPI. That's why we recommend installing HSSM directly from pip. It is guaranteed to be a version that passes all tests. :)

@zenkavi
Copy link
Author

zenkavi commented Aug 1, 2023

Sorry for the delay and thanks for the comments. I think I made the changes you suggested (including updating the tutorial directories). I'm happy to do any additional changes or feel free to merge and do more on your end. I also don't mind if you prefer rejecting this PR and writing a better/more comprehensive version yourself. This was just my attempt to share what works well for me so I thought I'd share in case there are others who might find it useful.

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