[BBS-266] Adapt Getting Started for DVC remote from Zenodo #267

pafonta · 2021-03-05T17:59:30Z

Fixes BBS-266.

TODO

~~Currently blocked by BBS-306.~~

Otherwise, after the tests are successful:

upload the TAR to Zenodo,
put the link in the instructions,
PR checklist below.

Description

Adapt the Getting Started from the README to use the DVC remote from Zenodo.

How to test?

1 - Getting Started

Follow the Getting Stared from the README after

git clone --depth 1 --branch bbs_266 https://github.com/BlueBrain/Search.git

The notebook at the end should be usable for search and mining.

2 - DVC

a) Setup

wget <fixme>
tar xf bbs_dvc_remote.tar.gz

git clone --depth 1 --branch v0.1.0 https://github.com/BlueBrain/Search.git

cd Search/

cp /path/to/prodigy/wheel .

dvc remote add --default local ../bbs_dvc_remote

b) Pulling

dvc pull

should retrieve all DVC tracked data and models successfully.

c) `pipelines/sentence_embedding`

After

docker build -f data_and_models/pipelines/sentence_embedding/Dockerfile -t <image_name> .
docker run -v /raid:/raid -it --rm --name <container_name> <image_name>
cd data_and_models/pipelines/sentence_embedding
dvc repro -f

the command

git diff

should show no change.

d) `pipelines/ner`

After

docker build -f data_and_models/pipelines/ner/Dockerfile -t <image_name> .
docker run -v /raid:/raid -it --rm --name <container_name> <image_name>
cd data_and_models/pipelines/ner
dvc repro -f

the command

git diff

should show no change but it will show these changes

--- a/data_and_models/pipelines/ner/dvc.lock
+++ b/data_and_models/pipelines/ner/dvc.lock
@@ -130,7 +130,7 @@ eval_disease:
   - path: ../../models/ner_er/model1
-    md5: 5757d1ea583e2ac6aa7e642e11b56325.dir
+    md5: a51a1dc7148255d3fb0e1f5b05079ec0.dir
     size: 100183520
     nfiles: 18
@@ -398,7 +398,7 @@ add_er_1:
   - path: ../../models/ner_er/model1
-    md5: 5757d1ea583e2ac6aa7e642e11b56325.dir
+    md5: a51a1dc7148255d3fb0e1f5b05079ec0.dir
     size: 100183520
     nfiles: 18

This issue isn't introduced by the current PR. This is already the situation on master.

Indeed, the issue is semi-reproducible (see #267 (comment)) outside this PR like this

git clone --depth 1 --branch v0.1.0 https://github.com/BlueBrain/Search.git
cd Search/
cp /path/to/prodigy/wheel .
docker build -f data_and_models/pipelines/ner/Dockerfile -t <image_name> .
docker run -v /raid:/raid -it --rm --name <container_name> <image_name>
dvc remote modify gpfs_ssh ask_password true
dvc remote modify gpfs_ssh user <user_name>
dvc pull
cd data_and_models/pipelines/ner
dvc repro -f
git diff dvc.lock

Checklist

This PR refers to an issue from the issue tracker.
Documentation and whatsnew.rst updated.
All CI tests pass.

FrancescoCasalegno · 2021-03-08T17:44:02Z

This issue isn't introduced by the current PR. This is already the situation on master.
Indeed, the issue is reproducible outside this PR like this

For the record, the issue mentioned here is semi-reproducible, in the sense that it is due to the non-deterministic behavior of a component of SpaCy which uses set. @jankrepl will fix this in #268 .

FrancescoCasalegno

Thank you very much @pafonta , this was really a great work!

EmilieDel

Really nice job! 🎉 Thanks a lot for doing this Zenodo thing @pafonta! I just did a small comment but can be ignored!

EmilieDel · 2021-03-09T16:01:39Z

README.md

+Third, keep track of the path to the working directory, the repository
+directory, and the data and models directory.


Maybe I could be better to say keep track of the paths of the working directory, the repository directory, and the data and models directory. However, it is really a minor comment and I am not confident about what I am suggesting!

The sentence is coming from #229 (see here lines 220-221).

@Stannislav was the author of this PR. Maybe he knows about what is valid English?

Could you have a look at this @Stannislav?

To me either way sounds fine :)

Lets keep it that way :)

Stannislav · 2021-03-09T16:54:15Z

docker/mining.sh

+# Commented to allow the use of a DVC remote from a different type than SSH.
+# ssh_check
+# Not usable in README as it works only when inside the `bbs_` containers.
+# dvc_pull_models


Could you please remind me how the server is going to find the NER models now?

The decision taken by the team yesterday to comment these lines puts the mining server in the same situation of the search server: dvc pull needs to have been run before. In the README, there is already a dvc pull happening before launching the mining server. Commenting the two lines doesn't therefore break something. For further discussions on the subject, there is the ticket BBS-306.

Yes, I was trying to make sure I understand the setting of the path to the models correctly. The mining server finds the models by reading the BBS_DATA_AND_MODELS_DIR environment variable. Here's my current understanding of setting this variable, could you maybe please check if it's correct?

With dvc_pull_models the models would be pulled somewhere locally on the container, so that the value of this variable needs to be configured to point to a local path, e.g.:

BBS_DATA_AND_MODELS_DIR=/src/data_and_models

After commenting out dvc_pull_models the models will be provided externally. So we need to docker run with a mount of the volume where these models are, e.g. with the -v /raid:/raid flag, and then configure the environment variable to point to a location on that volume:

BBS_DATA_AND_MODELS_DIR=/raid/<...>/projects/search/data_and_models

Is this understanding correct?

jankrepl

Good job!!

Stannislav · 2021-03-10T09:43:31Z

README.md

+Third, keep track of the path to the working directory, the repository
+directory, and the data and models directory.


To me either way sounds fine :)

[BBS-266] Adapt Getting Started for DVC remote from Zenodo

4709e22

pafonta added documentation Improvements or additions to documentation new feature New feature or request 🦉 dvc Add DVC data or models labels Mar 5, 2021

pafonta added 3 commits March 8, 2021 15:36

Allow other DVC remotes than a SSH one

0d9be2a

Update whatsnew

668414b

Add Zenodo badge and wget command

06f0871

pafonta marked this pull request as ready for review March 8, 2021 17:00

pafonta requested review from FrancescoCasalegno, Stannislav, jankrepl and EmilieDel March 8, 2021 17:06

FrancescoCasalegno approved these changes Mar 8, 2021

View reviewed changes

EmilieDel approved these changes Mar 9, 2021

View reviewed changes

Stannislav reviewed Mar 9, 2021

View reviewed changes

jankrepl approved these changes Mar 10, 2021

View reviewed changes

Stannislav approved these changes Mar 10, 2021

View reviewed changes

pafonta merged commit b6b0448 into master Mar 10, 2021

pafonta deleted the bbs_266 branch March 10, 2021 09:49

pafonta self-assigned this Mar 10, 2021

FrancescoCasalegno mentioned this pull request Mar 12, 2021

Docker files shouldn't rely exclusively on SSH for DVC #279

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BBS-266] Adapt Getting Started for DVC remote from Zenodo #267

[BBS-266] Adapt Getting Started for DVC remote from Zenodo #267

pafonta commented Mar 5, 2021 •

edited

Loading

FrancescoCasalegno commented Mar 8, 2021

FrancescoCasalegno left a comment

EmilieDel left a comment

EmilieDel Mar 9, 2021

pafonta Mar 9, 2021

pafonta Mar 10, 2021

Stannislav Mar 10, 2021

EmilieDel Mar 10, 2021

Stannislav Mar 9, 2021

pafonta Mar 9, 2021 •

edited

Loading

Stannislav Mar 10, 2021

pafonta Mar 10, 2021

jankrepl left a comment

Stannislav Mar 10, 2021

		Third, keep track of the path to the working directory, the repository
		directory, and the data and models directory.

[BBS-266] Adapt Getting Started for DVC remote from Zenodo #267

[BBS-266] Adapt Getting Started for DVC remote from Zenodo #267

Conversation

pafonta commented Mar 5, 2021 • edited Loading

TODO

Description

How to test?

1 - Getting Started

2 - DVC

a) Setup

b) Pulling

c) pipelines/sentence_embedding

d) pipelines/ner

Checklist

FrancescoCasalegno commented Mar 8, 2021

FrancescoCasalegno left a comment

Choose a reason for hiding this comment

EmilieDel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pafonta Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jankrepl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pafonta commented Mar 5, 2021 •

edited

Loading

c) `pipelines/sentence_embedding`

d) `pipelines/ner`

pafonta Mar 9, 2021 •

edited

Loading