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

169 fixed dependency versions #170

Merged
merged 18 commits into from
May 24, 2024
Merged

Conversation

mschwoer
Copy link
Collaborator

@mschwoer mschwoer commented May 23, 2024

Enforce fixed dependency versions, while concurrently supporting also a 'loose' install.
Consolidated and improved the installation docs.

@GeorgWa please check the versions in the requirements.txt .. also, some packages are missing there (like tqdm -> they come from some other alpha package, and maybe other could be removed (or moved to test/development requirements)?

@mschwoer mschwoer marked this pull request as ready for review May 23, 2024 12:17
Copy link
Collaborator

@jalew188 jalew188 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@GeorgWa GeorgWa left a comment

Choose a reason for hiding this comment

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

LGTM

README.md Outdated
```bash
pip install -e "."
pip install ".[stable]"
Copy link
Collaborator

Choose a reason for hiding this comment

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

What will be the default installation method if somebody types
pip install -e . or pip install alphadia?

I think it's important that this is stable and then we should also simplify the instructions.

Copy link
Collaborator

@GeorgWa GeorgWa May 24, 2024

Choose a reason for hiding this comment

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

Shouldn't this also be pip install -e ".[development,stable]"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did not find a way to support two requirements files (one with fixed, one with loose versions) while concurrently supporting a default. This means that you always need to specify the installation type (stable, loose). If this is omitted, no dependencies are being installed (which should result in errors quite early in the processing ;-))

For the second comment: I deliberately chose to separate the "user" from the "developer" install instructions. The dependencies should be organized as such as that alphaDIA should run without the "development" reqs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, I see this as quite a problem :/ can't we have a requirements.txt without suffix in the filename as fallback? Even if not perfect.

Copy link
Collaborator Author

@mschwoer mschwoer May 24, 2024

Choose a reason for hiding this comment

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

it's not about the file names, it's about this mapping in pyproject.toml

[tool.setuptools.dynamic]
# https://stackoverflow.com/a/73600610
# dependencies = {file = ["requirements/requirements.txt"]}
# no mandatory dependencies: user needs to specify "stable" or "loose" explicitly
optional-dependencies = { 
stable = { file = ["requirements/requirements.txt",] }, 
loose = { file = ["requirements/requirements_loose.txt",] }, 
test = { file = [ "requirements/requirements_test.txt",] }, 
development = { file = ["requirements/requirements_development.txt", "requirements/requirements_test.txt"
] }}

if you make the requirements.txt (with fixed versions) the default (dependencies = {file = ["requirements/requirements.txt"]}), then overriding with pip install -e .[loose] wont work.

We could try to turn it around, i.e. that pip install -e . installs loose, and "stable" must be explicitly stated.
However, I am not in favor of making "loose" the default. Can we not rely upon a user that uses "pip install" can also read off the correct commands from the Readme in case of an error? (we could also introduce a speaking error message: if e.g. alpharaw is not installed we inform about the correct way of installing via pip?)

anyway, I need to try this out first.

Copy link
Collaborator

@GeorgWa GeorgWa left a comment

Choose a reason for hiding this comment

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

LGTM

README.md Outdated

##### Setting up mono [MacOS/Linux only]
If you want to use `.raw` files on Thermo instruments alphaRaw is required, which depends on Mono. You can find the mono installation instructions [here](https://www.mono-project.com/download/stable/#download-lin). A detailed guide to installing alphaRaw can be found [here](https://github.com/MannLabs/alpharaw#installation).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's only cite the alphaRaw instructions here. I don't know why included the other instructions as they are actually not helpfull at all :D

@GeorgWa GeorgWa merged commit c4067b3 into development May 24, 2024
4 checks passed
@GeorgWa GeorgWa deleted the 169-fixed-dependency-versions branch May 24, 2024 16:06
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