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

Update of README. #170

Merged
merged 10 commits into from
Feb 2, 2024
Merged

Update of README. #170

merged 10 commits into from
Feb 2, 2024

Conversation

mikerobeson
Copy link
Collaborator

It's been a while since we've updated the README. My proposed changes are in this PR. This was motivated by this forum thread.

Copy link
Collaborator

@nbokulich nbokulich left a comment

Choose a reason for hiding this comment

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

Thanks @mikerobeson ! Indeed, this readme needs a good dusting off 😁

I have a few minor comments in-line, but overall my primary thoughts are about the install instructions. It would be nice to simplify the install instructions, as installation is not as complicated now as it was before.

RESCRIPt is already included in the shotgun distro, so a very easy installation instruction could just be to install that distro. We could drop the alternative options and just redirect to the install instructions for that distro.

I think RESCRIPt is also going to be added to the amplicon distro in the next release — is that correct, @lizgehret? Or in a future release.

README.md Outdated
@@ -20,7 +20,7 @@ First create a conda environment and install relevant dependencies:
conda create -y -n rescript
conda activate rescript
conda install \
-c conda-forge -c bioconda -c qiime2 -c https://packages.qiime2.org/qiime2/2023.5/tested/ -c defaults \
-c conda-forge -c bioconda -c qiime2 -c https://packages.qiime2.org/qiime2/2023.7/tested/ -c defaults \
Copy link
Collaborator

Choose a reason for hiding this comment

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

you install 2023.7 here, but then mention 2023.9 below. Why not just use the latest?

Copy link
Collaborator Author

@mikerobeson mikerobeson Jan 14, 2024

Choose a reason for hiding this comment

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

I wanted to highlight two ways to install RESCRIPt, as many of the recent versions can be installed as we've been doing. But not so with 2023.9...

I also figured, we'd not merge this PR until just before the next release anyway. This will serves as our scratch space. :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

okay good point, and I like the idea of keeping install instructions for pre-2023.9, but maybe we should change the header line accordingly

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated
qiime rescript --help
```

#### NOTE on installing RESCRIPt within `qiime2-amplicon-2023.9`:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this will already change in the next release...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, but will this work for 2023.9 though? If not, we'll have to keep this note for that version right?

Copy link
Collaborator

@nbokulich nbokulich left a comment

Choose a reason for hiding this comment

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

Thanks @mikerobeson !

See my two comments inline. I like your idea to include install instructions for 2023 releases. But I still find it a little confusing to show instructions for both 2023.7 and 2023.9.

Maybe starting 2023.9 as the primary installation instructions we recommend that people install the shotgun distro to use RESCRIPt? And they can also switch environments if they want to use this with amplicon data. That will change in the next release anyway, when I think that it will also be in the amplicon distro. So per your comment about merging this PR shortly before the next release I suggest that you already update the README to reflect the situtation in 2024.2. Does that sound okay?

README.md Outdated
@@ -20,7 +20,7 @@ First create a conda environment and install relevant dependencies:
conda create -y -n rescript
conda activate rescript
conda install \
-c conda-forge -c bioconda -c qiime2 -c https://packages.qiime2.org/qiime2/2023.5/tested/ -c defaults \
-c conda-forge -c bioconda -c qiime2 -c https://packages.qiime2.org/qiime2/2023.7/tested/ -c defaults \
Copy link
Collaborator

Choose a reason for hiding this comment

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

okay good point, and I like the idea of keeping install instructions for pre-2023.9, but maybe we should change the header line accordingly

README.md Outdated
@@ -31,17 +31,36 @@ pip install git+https://github.com/bokulich-lab/RESCRIPt.git
```

### Option 2: Install within QIIME 2 environment
Copy link
Collaborator

Choose a reason for hiding this comment

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

say "before version 2023.9"?

Copy link
Collaborator Author

@mikerobeson mikerobeson Jan 18, 2024

Choose a reason for hiding this comment

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

I assume the install instructions for 2024.2 are going to be largely similar to 2023.2 - 2023.7 right? That is pointing to a .../202X.Y/tested/ folder? If so, I think labelling as 'before' or 'after' 2023.9 might be confusing, as that is the only odd ball. Which is why I set it up as a note for 2023.9. 🤷 I can add the note about installing the shotgun distro too.

Also, this makes me think... should we add instructions for older versions, 2022.8 and earlier? Or would that just make things more confusing? I figure no, as folks can ask for help with older versions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh wait I forgot, pre- 2023.9 the tested folders will be https://packages.qiime2.org/qiime2/2023.7/tested/ whereas 2024.2 will be: https://packages.qiime2.org/qiime2/2024.2/amplicon/released/. So, the pre- post labeling will work. :-)

@lizgehret
Copy link
Collaborator

Thanks @mikerobeson ! Indeed, this readme needs a good dusting off 😁

I have a few minor comments in-line, but overall my primary thoughts are about the install instructions. It would be nice to simplify the install instructions, as installation is not as complicated now as it was before.

RESCRIPt is already included in the shotgun distro, so a very easy installation instruction could just be to install that distro. We could drop the alternative options and just redirect to the install instructions for that distro.

I think RESCRIPt is also going to be added to the amplicon distro in the next release — is that correct, @lizgehret? Or in a future release.

Sorry for the delayed response @nbokulich! Yes, we're aiming to get RESCRIPt in the amplicon distro in this upcoming release - which I think should be doable because @hagenjp will be working on moving all of the types/formats/etc from q2-types-genomics over to q2-types.

@mikerobeson
Copy link
Collaborator Author

I just put up another draft of the README. Still a work in progress, but wanted to put forth some ideas on how to clarify installation, etc. :-)

README.md Outdated
xmltodict 'q2-types-genomics>2023.2' ncbi-datasets-pylib
conda activate qiime2-amplicon-2024.2
conda install -c conda-forge -c bioconda -c qiime2 \
-c https://packages.qiime2.org/qiime2/2024.2/amplicon/tested/ \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
-c https://packages.qiime2.org/qiime2/2024.2/amplicon/tested/ \
-c https://packages.qiime2.org/qiime2/2024.2/amplicon/passed/ \

We've changed our package dir structure slightly - so the closest equivalent to the tested channel is now the passed channel. That being said, using the released channel will also work since this is essentially just a wholesale install of the latest amplicon release.

@nbokulich
Copy link
Collaborator

Hey @mikerobeson I think that the number of install options is getting a bit overwhelming. I think that options 1 and 2 are enough. Maybe move the other options to a side document? And we could just drop a link to that in the README. This would mostly be for posterity and probably not useful to anyone after another release or two.

@mikerobeson
Copy link
Collaborator Author

mikerobeson commented Jan 18, 2024

number of install options is getting a bit overwhelming

I agree @nbokulich. I really like your idea of a side document. I can just link to that and say something like, "For additional notes on installing other versions of RESCRIPt please see the following document".

@mikerobeson
Copy link
Collaborator Author

Just pushed a new draft update of the README.md, and added the supplementary file install-prior-versions.md.

@mikerobeson
Copy link
Collaborator Author

Should we remove the lint-build-test badge? Or is there a way to fix it or point to something else?

@nbokulich
Copy link
Collaborator

Hey @mikerobeson thanks for the updates! LGTM. Are we ready to merge or do we wait for 2024.2 to release?

Yeah thanks for spotting the broken badge, the badge should have been changed when the CI was updated. I will investigate this and open a separate PR for the sake of simplicity/clarity, instead of giving you more homework on this PR! 😂

@mikerobeson
Copy link
Collaborator Author

I think this is good to go for merging.

@mikerobeson
Copy link
Collaborator Author

I'll accept and merge your badge PR and then update this PR.

@mikerobeson
Copy link
Collaborator Author

mikerobeson commented Jan 31, 2024

Okay @nbokulich. This should be good to merge once the checks pass. :-)

@nbokulich
Copy link
Collaborator

Hey @mikerobeson sorry, still not ready to merge, I caught something else.

At first I was going to suggest that we wait to merge until 2024.2 is released later this month, as some of the links that you give in the instructions (e.g., https://docs.qiime2.org/2024.2/install/native/) are not yet active and only will be after the release.

But then I realized that those instructions do not make sense anyway, as RESCRIPt will be part of the 2024.2 amplicon distro.

So I think that the installation instructions should instead have the following options/structure:

  1. "Install as part of a QIIME 2 distribution": Refer to the QIIME 2 documentation for the latest installation instructions and install the q2-shotgun distribution. (then once 2024.2 is released we can add the amplicon distro to this list). I would just point to docs.qiime2.org so that the link does not become stale if pointing to a release-specific install doc.
  2. Minimal RESCRIPt environment (fine as is, but let's swap the order)
  3. Prior versions of RESCRIPt. (fine as is, but maybe let's say "Prior versions of RESCRIPt / QIIME 2"? As these would be the instructions if trying to install in a pre-2024.2 env)

Does that sound okay? Sorry for the 11th-hour changes!

@mikerobeson
Copy link
Collaborator Author

That all makes sense to me @nbokulich! 👍

@mikerobeson
Copy link
Collaborator Author

mikerobeson commented Feb 1, 2024

Hey @nbokulich, I realized that I need to update the commands users to install the "Minimal RESCRIPt environment". I am unable to get this to work. This is the closest to success I've been able to get:

conda install \
  -c https://packages.qiime2.org/qiime2/2023.9/shotgun/passed/ \
  -c qiime2 -c conda-forge -c bioconda -c defaults \
  qiime2 q2cli q2templates q2-types q2-types-genomics q2-longitudinal q2-feature-classifier \
  "pandas>=0.25.3" xmltodict ncbi-datasets-pylib

I've tried a variety of things, like switching the channel order, altering allowable versions, etc. But I keep getting this error (when using mamba to install):

q2-types-genomics is not installable because it requires
└─ qiime2 2023.9.* , which conflicts with any installable versions previously reported.

I'm not sure of the best way to go about resolving this. Any ideas? 🤷‍♂️

@nbokulich
Copy link
Collaborator

maybe we just drop the minimal install instructions? We could figure out how to add RESCRIPt to the "tiny" distro, but otherwise rescript depends on a few plugins so installing as part of a distro is simplest and probably best.

@mikerobeson
Copy link
Collaborator Author

mikerobeson commented Feb 1, 2024

I think I figured it out! If I simply add both shotgun and amplicon 'passed' channels it works:

conda create -y -n rescript
conda activate rescript
conda install \
  -c https://packages.qiime2.org/qiime2/2023.9/shotgun/passed/ \
  -c https://packages.qiime2.org/qiime2/2023.9/amplicon/passed/ \
  -c conda-forge -c bioconda -c qiime2 -c defaults \
  qiime2 q2cli q2templates q2-types q2-types-genomics q2-longitudinal q2-feature-classifier \
  "pandas>=0.25.3" xmltodict ncbi-datasets-pylib

pip install git+https://github.com/bokulich-lab/RESCRIPt.git

Perhaps a middle ground? That is, I can just append these "minimal environment" instructions to the install-prior-versions.md document? 🤔

@mikerobeson
Copy link
Collaborator Author

mikerobeson commented Feb 1, 2024

I updated another draft, leaving the "minimal install" option in the README. But if you really think we should drop it, I'll move it to the supplementary instructions. I just feel we should provide a minimal option somewhere.

Copy link
Collaborator

@nbokulich nbokulich left a comment

Choose a reason for hiding this comment

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

Thanks @mikerobeson ! A couple small tweaks, see inline.

If you want the minimal install that's fine, we can keep it. My feeling is just that we basically install half of the amplicon distro in the minimal install, so this is maybe obsolete now 🤷 but I don't have a strong objection if you think it's still useful.

README.md Outdated
"pandas>=0.25.3" xmltodict ncbi-datasets-pylib
```

Install source:

```
pip install git+https://github.com/bokulich-lab/RESCRIPt.git
Copy link
Collaborator

Choose a reason for hiding this comment

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

RESCRIPt should now be conda installable via the same channels above, so this should not be necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Doh! 🤦

Install source:

```
pip install git+https://github.com/bokulich-lab/RESCRIPt.git
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above — should conda install

@mikerobeson
Copy link
Collaborator Author

@nbokulich, I moved the minimal install to the supplement. I also removed the pip command and updated the conda install to include rescript. It all seems to work.

Copy link
Collaborator

@nbokulich nbokulich left a comment

Choose a reason for hiding this comment

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

Thanks @mikerobeson ! Looks great! We are now ready to merge 😎

@nbokulich nbokulich merged commit a19d106 into bokulich-lab:master Feb 2, 2024
4 checks passed
@mikerobeson mikerobeson deleted the update-readme branch February 2, 2024 15:14
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