-
Notifications
You must be signed in to change notification settings - Fork 12
Conversation
It seems that
should instead be
in the test instructions. |
When building the NER image I get the following error on the line
The internet suggests to use |
@jankrepl I have rebuild the image right now from scratch (using a fresh clone of this repo) and I cannot reproduce the error with Maybe a fresh |
Hello @FrancescoCasalegno ! I cannot run
but I get the error
|
I got exactly the same error. |
As discussed, I got this error when using the most recent |
Thanks, I now also see the error! This seems to be a known issue, and reading this SO post as well as this GH issue I think we only have 3 options:
I think the third option seems the least ugly, I am now testing if it works fine. |
Yes, I went for 3. locally and it worked. Anyway, I think we should pin the |
@jankrepl and @pafonta — I always did It seems that this is a known issue that should be fixed in the future. I updated the instructions to suggest a testing workflow that should run smoothly using |
There was a problem hiding this 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 work!
I have followed the new testing instructions. I was able to reproduce the expected results (i.e. no output for git diff
after dvc repro -f
).
I don't approve yet because I have the 2 questions and the suggestion below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you for the work!
Co-authored-by: Pierre-Alexandre Fonta <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks!
Fixes BBS-292.
Description
requirements.txt
. This change improves the reproducibility of results with DVC. Before this, we were just using the dependencies specified insetup.py
which means that there was no guarantee on the version of each dependency used to run the DVC pipelines and results could have slightly changed.prodigy
version to fix this error.dvc repro
(anddvc push
to update the remote storage)..dvc/cache
to only4.4 GiB
.How to test?
cp /raid/prodigy_downloads/1.10.7/prodigy-1.10.7-cp36.cp37.cp38-cp36m.cp37m.cp38-linux_x86_64.whl path/to/Search
ner
andsentemb
pipelines. You have two options.bbs_dvc_ner:v0.1.0
andbbs_dvc_sentemb:v0.1.0
. In the following I assume you use this option—otherwise replace the image names in the following commands!docker build -f data_and_models/pipelines/ner/Dockerfile -t <ner_image_name> .
docker build -f data_and_models/pipelines/sentence_embedding/Dockerfile -t <sentemb_image_name> .
ner
pipeline.dvc pull
docker run --rm -it <your_options> --name <ner_container_name> bbs_dvc_ner:v0.1.0
cd data_and_models/pipelines/ner/
dvc repro
(ordvc repro -f
if you have some time and want to be 100% sure)git diff
should be emptysentemb
pipeline.dvc pull
(if you run thener
pipeline already, then this step can be skipped)docker run --rm -it <your_options> --name <sentemb_container_name> bbs_dvc_sentemb:v0.1.0
cd data_and_models/pipelines/sentence_embedding/
dvc repro
(ordvc repro -f
if you have some time and want to be 100% sure)git diff
should be emptyChecklist
(if it is not the case, please create an issue first).
whatsnew.rst
updated.(if needed)