ivadomed documentation modifications

[copperwiring]

Checklist

GitHub

• I've given this PR a concise, self-descriptive, and meaningful title
• I've linked relevant issues in the PR body
• I've applied the relevant labels to this PR
• I've assigned a reviewer

PR contents

• I've consulted ivadomed's internal developer documentation to ensure my contribution is in line with any relevant design decisions
• I've added relevant tests for my contribution
• I've updated the relevant documentation for my changes, including argparse descriptions, docstrings, and ReadTheDocs tutorial pages

Description

In ivadomed installlation doc (https://ivadomed.org/en/latest/installation.html), under Approach 2, the second step mentions:

pip install -r requirements.txt

However, there is no mention of cloning the repo which is where the requirement.txt exists. Though git clone is an obvious step, it can make the installation instructions complete.

Linked issues

Fixes #849

[jcohenadad]

there are also issues with the missing pip install -e . #844 which could be address in this PR (the other issue deals with this exact same part of the documentation so it makes review process more efficient-- ie: no need for the review team to go through the same file twice while doing the review-- also it will make it possible to review a comprehensive install doc)

[copperwiring]

there are also issues with the missing pip install -e . #844 which could be address in this PR (the other issue deals with this exact same part of the documentation so it makes review process more efficient-- ie: no need for the review team to go through the same file twice while doing the review-- also it will make it possible to review a comprehensive install doc)

Thanks, @jcohenadad . Fair point. I wasn't sure if I should open individual issues or keep them same. May be I can give a more general title of the title like 'ivadomed doc modifications'? As for issue #844 , I did not have much success with conda on my local system since the conda env was not discoverable.

Hence, I ended up using virtualenv both locally and on compute canada. @dyt811 is aware of this issue I had. I am not sure how can I update the doc since I could not get it working with conda. I mean, I can add the steps but I can't test it hence my hesitation. vietualenv has been kind to me and hence my documentation updates are all related to the same.

[coveralls]

Pull Request Test Coverage Report for Build 1187236723

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 70.698%

---

Totals
Change from base Build 1177977213:	0.0%
Covered Lines:	4285
Relevant Lines:	6061

---

💛 - Coveralls

[jcohenadad]

May be I can give a more general title of the title like 'ivadomed doc modifications'?

👍

[jcohenadad]

As for issue #844 , I did not have much success with conda on my local system since the conda env was not discoverable.

hum... weird... maybe another bug? i was able to have it work on my end. Maybe a full terminal output would be more informative to find out the source of the bug?

[copperwiring]

As for issue #844 , I did not have much success with conda on my local system since the conda env was not discoverable.

hum... weird... maybe another bug? i was able to have it work on my end. Maybe a full terminal output would be more informative to find out the source of the bug?

Oh, the screenshot is the full terminal out. Here under Approach 1, step 1 and step 2 is what I did (as seen in screenshot) but conda env was not found. There was error. it's just that it didn't find the conda env to activate

[jcohenadad]

As for issue #844 , I did not have much success with conda on my local system since the conda env was not discoverable.

hum... weird... maybe another bug? i was able to have it work on my end. Maybe a full terminal output would be more informative to find out the source of the bug?

Oh, the screenshot is the full terminal out.

from the terminal we cannot see what git commit of ivadomed you used, what version of conda, etc.

Here under Approach 1, step 1 and step 2 is what I did (as seen in screenshot) but conda env was not found. There was error. it's just that it didn't find the conda env to activate

the URL brings me there:

[copperwiring]

the URL brings me there:

I apologize for the mixup in URLs. This is the correct URL: https://ivadomed.org/en/latest/installation.html (please step 1 and step 2 under Approach 1).

---

from the terminal we cannot see what git commit of ivadomed you used, what version of conda, etc.

It is very much possible that it is an issue on my conda env. I will double check it tomorrow and update accordingly.

[hermancollin]

I tried installing with approach 1 and it failed on my end too.

Step 1:

$ conda env create --file environment.yml                                                                                                           
EnvironmentFileNotFound: '/home/herman/Documents/NEUROPOLY_21/crypte/environment.yml' file not found

And of course the file isn't found, because the repo hasn't been cloned yet (instructions are missing this preliminary step). My understanding is that the environment.yml file is the one from the ivadomed repo. Also, step 1 is called Create new Conda Env called IvadoMedEnv but the name isn't even specified in the command itself. Instead, maybe first clone the repo using:

git clone https://github.com/neuropoly/ivadomed.git
cd ivadomed

(btw the doc specifies in approach 2 using git clone git@github.com:ivadomed/ivadomed.git which fails on my end)

then, create the conda virtual env and specify the right name:

conda env create -f environment.yml -n IvadoMedEnv
conda activate IvadoMedEnv

This is how I got it to work on my end. Moreover, as mentioned by @jcohenadad in #844, the pip install step is missing, for example, if we want to install from source, this last step is required:

pip install -e .

Now it works on my side. @copperwiring if you try this instead does it work on your end?

[copperwiring]

And of course the file isn't found, because the repo hasn't been cloned yet (instructions are missing this preliminary step). My understanding is that the environment.yml file is the one from the ivadomed repo. Also, step 1 is called Create new Conda Env called IvadoMedEnv but the name isn't even specified in the command itself. Instead, maybe first clone the repo using:

Thank you for checking @hermancollin . However I had cloned the repo :) In the snapshot you will see the folder name I am in (ivadomed). It recognized the yml file too. It was definitely my conda issue because it worked for me today morning 😄

I agree we should add the following to complete the steps

pip install -e .

---

(btw the doc specifies in approach 2 using git clone git@github.com:ivadomed/ivadomed.git which fails on my end)

I am curious about this. It should have worked. What's the error?

[hermancollin]

And of course the file isn't found, because the repo hasn't been cloned yet (instructions are missing this preliminary step). My understanding is that the environment.yml file is the one from the ivadomed repo. Also, step 1 is called Create new Conda Env called IvadoMedEnv but the name isn't even specified in the command itself. Instead, maybe first clone the repo using:

Thank you for checking @hermancollin . However I had cloned the repo :) In the snapshot you will see the folder name I am in (ivadomed). It recognized the yml file too. It was definitely my conda issue because it worked for me today morning

Yes I noticed you had indeed cloned the repo because the "environment.yml" was found, from what I see in your screenshot, but don't you agree that it should be mentioned in the installation steps to first clone the repo?

(btw the doc specifies in approach 2 using git clone git@github.com:ivadomed/ivadomed.git which fails on my end)

I am curious about this. It should have worked. What's the error?

~/Documents/NEUROPOLY_21/dummy » git clone git@github.com:ivadomed/ivadomed.git                                                                                                     herman@tank
Cloning into 'ivadomed'...
git@github.com: Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

I should mention I don't have permissions for this repo (I am not a member of ivadomed org). However,

git clone https://github.com/neuropoly/ivadomed.git

works for me as the repo is public. If the documentation is addressed towards people who aren't necessarily members of this org, maybe this last command should be used instead (same command as the one suggested in the readme)?

[copperwiring]

Yes I noticed you had indeed cloned the repo because the "environment.yml" was found, from what I see in your screenshot, but don't you agree that it should be mentioned in the installation steps to first clone the repo?

I agree, it should be mentioned. I was hesitant to touch that section previously because I couldn't get conda working but I will push the changes

I am curious about this. It should have worked. What's the error?

~/Documents/NEUROPOLY_21/dummy » git clone git@github.com:ivadomed/ivadomed.git                                                                                                     herman@tank
Cloning into 'ivadomed'...
git@github.com: Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

I should mention I don't have permissions for this repo (I am not a member of ivadomed org). However,

git clone https://github.com/neuropoly/ivadomed.git

works for me as the repo is public. If the documentation is addressed towards people who aren't necessarily members of this org, maybe this last command should be used instead (same command as the one suggested in the readme)?

That's a very very good point and thank you for pointing this point. This will and needs to changed. Thank you again @hermancollin .

[dyt811]

Great points. A bit context on the core issues at hand. Pre #757, the installation instruction actually does NOT require cloning of the repo (user manually install PyTorch and can use PIP to install release version etc). Post #757, we tried to bundle that into Conda/PyTorch requirements.txt and environment.yml to hopefully have one step installation for dev, users etc, but my pigeon brain conveniently forgot those files don't exists until your clone repo so didn't include that. Ideally, cloning repo is the very first step and is common to both VENV and conda environmental setup. Thank you for correcting that.

So I further tested conda create from base environment on Windows, from the latest master branch and it takes about 3 to 4 minutes to fully resolve and finish installing and I am on conda 4.8.3 on Windows 10. https://drive.google.com/file/d/17YqWYwsonUGqN50gz9Ic3GB-zZX60cO5/view?usp=sharing

A NEW PR has just been issued to allow EMPIRICAL CI based validation of conda environment installation time #855. You can see that across all OSes here (https://github.com/ivadomed/ivadomed/actions/runs/1021340338), using miniconda should not take longer than 10 minutes. I believe there might be additional confounding issues that are causing the LONG time to solve conda as this has been reported several time. I am going to open an issue to centralize the data on long solving time and see if we spot any particular patterns that is hoping up. BTW, that PR also provides Conda dev environment setup (needed for CI).

[hermancollin]

With these modifications Approach 1 works fine on my side.

[copperwiring]

Thanks everyone. I have made the suggested changes including the one suggested here. @hermancollin - though you seem to be able to approve the changes, it says At least 1 approving review is required by reviewers with write access.. Do you not have access to it? (CC: @kousu @dyt811)

[kousu]

Thanks everyone. I have made the suggested changes. @hermancollin - though you seem to be able to approve the changes, it says At least 1 approving review is required by reviewers with write access.. Do you not have access to it? (CC: @kousu @dyt811)

Invited, y'all.

(I kind of think everyone should be in everything but the current plan is to keep teams somewhat separate to keep the notification noise down. But if you're doing reviews for ivadomed then you're part of ivadomed I guess.)

[copperwiring]

Thanks for the promptness @kousu !

[jcohenadad]

i would remove-- i would minimize the references to CC-- ivadomed is an open source project dedicated to the international community-- people outside of canada have no clue what CC is so it gives the impression from "outsiders" that they are penetrating a closed circle and we don't want to give that impression-- we want to be inclusive

[copperwiring]

Hi @jcohenadad , may I suggest keeping this line. When I started as a newcomer, this line was very helpful for me since I used conda a lot. This helped me to directly use venv for installation purpose.

[jcohenadad]

1. it is not the right place to list all computer clusters policies (if we start with that should we also list the policy of clusters in France, Germany, etc.?). The info should be moved somewhere else (eg internal developers at neuropoly, etc.)
2. conda can be used on CC (i use it). It is just not recommended/supported.

[copperwiring]

Thank you @jcohenadad . My bad, as I understood that conda can not be used on CC but I stand corrected. I will remove this link and it can later be added to internal document. Thank you for the suggestion.

[copperwiring]

The reference has now been removed. Thank you!

[copperwiring]

Took a while to get back to it but all the suggestions and errors are fixed here :)

[copperwiring]

@jcohenadad @hermancollin I am cleaning out old PRs which I feel can now be closed. @hermancollin you did approve the changes but had recommended other changes later. They have now been completed. @jcohenadad I have fixed the compute canada reference as suggested.

Please let me know if things look good now.

[hermancollin]

@copperwiring Yes! Everything LGTM, except maybe you missed one small thing @jcohenadad suggested: removing the CC note in line 71. Did you decide to keep it? If so, let me know and I can approve merging right away

edit: @copperwiring ok as you mentioned, I did approve the changes but maybe you'll want to confirm line 71 here stays before merging. Apart from that I think this PR is ready to merge. Thanks for taking some time to fix this, the new installation procedure makes much more sense.

[copperwiring]

Thanks, @hermancollin Thanks for approving. I realised I had confused the comment about removing CC with another comment (add link to CC).

I just responded to Julien there and I think it would be nice to keep that line because it is important for people who are used to using conda. We may be able to relocate it to the top before we start discussing the installation step? I would wait to hear back on this, before merging this.

Thank you.

Update: As per the recommendation here, the line has been removed.