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

Docker local changes #711

Merged
merged 7 commits into from
Sep 21, 2022
Merged

Docker local changes #711

merged 7 commits into from
Sep 21, 2022

Conversation

ackagel
Copy link
Contributor

@ackagel ackagel commented Sep 15, 2022

Purpose

Allows users to update+reinstall ark codebase within docker with -u flag. closes #703

Implementation

Adds the ARK_UPDATE environment variable, which is set to 0 or 1 depending on if the -u or --update flag is set. If true, the docker will reinstall ark-analysis before running the jupyter lab command. This is done by replacing the CMD jupyter lab ... line in Dockerfile with a call to the new start_jupyter.sh script.

Remaining issues

I think I can neatly fit #710 in here too, but this is ready for testing in the mean time.

@ackagel
Copy link
Contributor Author

ackagel commented Sep 15, 2022

@cliu72 here's the updatable docker stuff. There isn't a docker image uploaded for this one at the moment; I'll make one so you can just pull that instead of building this locally. I'll let you know when that's up so you can confirm this updates ark code with that -u flag (I tested it on my end and it worked ok, but just a double check).

RUN apt-get install -y r-cran-igraph r-bioc-biocgenerics r-bioc-consensusclusterplus r-cran-dplyr r-cran-ggforce r-cran-ggplot2 r-cran-ggpubr r-cran-ggrepel r-cran-magrittr r-cran-pheatmap r-cran-rlang r-cran-rtsne r-cran-tidyr r-cran-xml r-cran-scattermore
#install flowsom dependency requirements (eye-roll)
RUN apt-get install -y r-cran-rcppparallel r-bioc-biobase r-cran-matrixstats r-cran-png r-cran-jpeg r-cran-interp r-cran-mass r-bioc-graph r-bioc-rbgl r-cran-scales r-cran-digest r-cran-bh r-cran-rcpparmadillo r-cran-jsonlite r-cran-base64enc r-cran-plyr r-bioc-zlibbioc r-cran-hexbin r-cran-gridextra r-cran-yaml r-bioc-rhdf5lib r-cran-corpcor r-cran-runit r-cran-tibble r-cran-xml2 r-cran-tweenr r-cran-gtable r-cran-polyclip r-cran-tidyselect r-cran-withr r-cran-lifecycle r-cran-rcppeigen

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh my god.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes i endured pain

Copy link
Contributor

Choose a reason for hiding this comment

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

lol

Copy link
Contributor

@cliu72 cliu72 left a comment

Choose a reason for hiding this comment

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

Just tested it and it works for me!

@ngreenwald
Copy link
Member

Will you also test modifying the ark codebase with jupyter launched, and see what %magic% command or whatever needs to be run within the notebook for the updated changes?

@ngreenwald
Copy link
Member

Testing this is what I meant:
cd /opt/ark-analysis && pip install

@ackagel
Copy link
Contributor Author

ackagel commented Sep 16, 2022

@cliu72 awesome! do you mind also running some R and/or pixel clustering stuff, just to make sure it works? I changed the way flowsom gets installed (for speed & less-"compiler error debugging" reasons), and just want to make sure everything works as intended.

Mostly made those changes because travis ran into a compiler error while trying to get this docker built that looked like it might bog down future docker builds on travis. Bonus from that adventure, the docker compilation time is down from ~1hr to ~20mins

@cliu72
Copy link
Contributor

cliu72 commented Sep 17, 2022

Will you also test modifying the ark codebase with jupyter launched, and see what %magic% command or whatever needs to be run within the notebook for the updated changes?

Yup tested this already. The specific commands are:
cd /opt/ark-analysis
pip install .

(double ampersand doesn't work)

Also @ackagel, I didn't actually need the -u tag to make this work. Without it, the above commands still work to make the code editable.

Copy link
Member

@ngreenwald ngreenwald left a comment

Choose a reason for hiding this comment

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

I think the -u flag basically does that automatically. So for non technical users, they can just run that command if we've changed the code but not made a new release, and they'll get the updated version. For developers, they can run that command the first time to incorporate their changes, and then for subsequent changes with the docker still open, use that command within the notebook. Is that correct Adam?

How long did it take when you ran that in the notebook?

@cliu72
Copy link
Contributor

cliu72 commented Sep 19, 2022

Oh I see. So using the -u flag should pull the latest changes in ark, and running the cd/pip commands will pull any local changes the user makes. Is that right? If that's the case, I tested the second part of that (cd/pip part) and it works (not sure if you wanted me to test the first part of that too? not sure how to do that)

Running the pip install command in jupyter takes ~20 seconds.

Tested pixel clustering and it seems fine. Just a note - Alex created our own version of the FlowSOM package and edited it a bit. I don't think his changes are super obvious when testing, so I didn't confirm whether our version of FlowSOM is being used. Just wanted to confirm that with your changes, you're using our version of FlowSOM and not installing from Bioconductor.

@ackagel
Copy link
Contributor Author

ackagel commented Sep 20, 2022

The -u flag doesn't pull changes; users still need to run git pull. The -u flag just runs the cd/pip commands automatically, before opening up jupyter.

I can confirm that we're using our version of FLowSOM, not Bioconductor's.

@ngreenwald sounds like we're all good here, so I'll update the branch and merge it in post-standup

@ackagel
Copy link
Contributor Author

ackagel commented Sep 20, 2022

@ngreenwald just need you to approve to unblock merging

Copy link
Member

@ngreenwald ngreenwald left a comment

Choose a reason for hiding this comment

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

Looks good

@ngreenwald ngreenwald merged commit ded6912 into main Sep 21, 2022
@ngreenwald ngreenwald deleted the docker-local-changes branch September 21, 2022 01:58
@srivarra srivarra added the bug Something isn't working label Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Newest start_docker.sh file doesn't allow for local changes to code
4 participants