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 README installation instructions to be more user friendly #89 #91

Merged
merged 6 commits into from
Mar 28, 2024

Conversation

rkstu
Copy link
Contributor

@rkstu rkstu commented Mar 3, 2024

This is related to GeomScale/dingo issue #83

Instruction changes:

  • Make sure the Python version is 3.8 workflows

  • Install a specific version of poetry workflows

curl -sSL https://install.python-poetry.org | python3 - --version 1.3.2
  • Install the dependencies for the PySPQR library using
sudo apt-get update -y
sudo apt-get install -y libsuitesparse-dev

Installation Instructions in GitHub Codespaces added in readme file
For a quick and hassle-free Dingo project trial, reference on how to setup python version 3.8.x in GitHub Codespaces added, from where user can follow rest of the installation steps.

Tutorial for local environment added
A how to use dingo in local env file added in dingo/tutorials/dingo_walkthrough.ipynb. This is a smaller version of colab tutorial. User should be able to run all cells after complete installation.

Unit Tests
All unit tests passed without issues(OK status), except for the test tests/rounding.py, which returned an error.

rounding_test_failed

output: tests/rounding.py

I followed these steps on my computer (Ubuntu 24.04) and GitHub Codespaces, and everything worked well for installing the Dingo project. If you have any suggestions or changes, please let me know.

Copy link
Contributor

@vfisikop vfisikop left a comment

Choose a reason for hiding this comment

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

Thanks! I would like to take some time to review the tutorial.

README.md Outdated
You can have a look at our [Google Colab notebook](https://colab.research.google.com/github/GeomScale/dingo/blob/develop/tutorials/dingo_tutorial.ipynb)
on how to use `dingo`.

#### In local environment
You can have a look at `dingo/tutorials/dingo_walkthrough.ipynb`. Now you should be able to run all cells.
Copy link
Contributor

Choose a reason for hiding this comment

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

It is easier if we include a link to the tutorial file

Copy link
Contributor Author

@rkstu rkstu Mar 5, 2024

Choose a reason for hiding this comment

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

dingo/tutorials/dingo_walkthrough.ipynb is intended for tutorial purpose in local environment after complete installation.

Not sure about "include a link to the tutorial file", should I create a colab notebook or provide a link to the walkthrough file in GitHub?

Copy link
Contributor

Choose a reason for hiding this comment

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

I propose to minimize the review process and leave the tutorial as a separate PR. So this PR will only include the changes to README. Do you agree?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I am not convinced that we need a "local" version of the tutorial. Isn't the current tutorial enough to execute locally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current tutorial should be sufficient for local execution. My initial thought was to have a separate file specifically for the local project demo, but on second thought, the existing tutorial works perfectly. I've gone ahead and removed the "dingo_walkthrough" file to avoid any confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, Let's keep this PR focused on the README changes and submit the tutorial in a separate PR. This will help keep the review process streamlined.

README.md Outdated
```
apt-get install libsuitesparse-dev

To install the Python dependencies, [Poetry](https://python-poetry.org/).
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens with other poetry versions? Have you tried?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tried multiple combinations of Python and poetry versions and followed the same installation instructions, below is the output of the poetry install command

Python 3.10.13 and Poetry (version 1.8.2) [If installed using pip install poetry]
image

Python 3.8.18 and Poetry (version 1.8.2) [If installed using pip install poetry]
image

Python 3.10.13 and Poetry (version 1.3.2)
image

Python 3.8.18 and Poetry (version 1.3.2)
image

README.md Outdated

**Note:** If you are using `GitHub Codespaces`. Start [here](https://docs.github.com/en/codespaces/setting-up-your-project-for-codespaces/adding-a-dev-container-configuration/setting-up-your-python-project-for-codespaces) to set the python version. Once your Python version is `3.8.x` you can start following the below instructions.

Clone dino repository
Copy link
Contributor

Choose a reason for hiding this comment

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

dino --> dingo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in readme file

## Installation

**Note:** Python version should be 3.8.x. You can check this by running the following command in your terminal:
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens with other Python versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tried multiple combinations of Python and poetry versions and followed the same installation instructions, below is the output of the poetry install command

Python 3.10.13 and Poetry (version 1.8.2) [If installed using pip install poetry]
image

Python 3.8.18 and Poetry (version 1.8.2) [If installed using pip install poetry]
image

Python 3.10.13 and Poetry (version 1.3.2)
image

Python 3.8.18 and Poetry (version 1.3.2)
image

Copy link
Contributor

Choose a reason for hiding this comment

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

great, could you please list all the possible choices and the possible solutions/workarounds in the README.
e.g. Python 3.8.18, Poetry 1.3.2 -- OK
Python 3.10.13, Poetry 1.3.2 -- fail

BTW why the second case above fails? Is there a workaround?

Copy link
Contributor Author

@rkstu rkstu Mar 22, 2024

Choose a reason for hiding this comment

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

I tried with multiple other combinations of Python and Poetry versions, but only Python 3.8 with Poetry 1.3.2 seems to work consistently yet, while other combinations like Python 3.10.13 with Poetry 1.3.2 fails due to dependency compatibility issues. This is likely because some libraries haven't been updated for other python versions such as Python 3.10 yet. I tried workarounds like specifying broader version ranges (e.g., numpy==^1.20.1) in the pyproject.toml file, and this doesn't seems to help.

README.md Outdated
```bash
python --version
```
If you have a different version of Python installed, you'll need to install it or update-alternatives [start here](https://www.youtube.com/watch?v=w7v4CZt1po8).
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer a Python site or blog and not a youtube link

Copy link
Contributor Author

@rkstu rkstu Mar 5, 2024

Choose a reason for hiding this comment

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

I've revised the Readme file to include installation instruction blog posts similar to those found in the video.

Python installation: https://linuxize.com/post/how-to-install-python-3-8-on-ubuntu-18-04/
Update default Python version: https://linuxhint.com/update_alternatives_ubuntu/

If you have any suggestions or changes, please let me know.

"metadata": {},
"source": [
"Now, you can load a metabolic model. \n",
"Currently, `dingo` may get either a `.json` or a '.mat` model format."
Copy link
Contributor

Choose a reason for hiding this comment

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

.mat is not correctly rendered.

Copy link
Contributor Author

@rkstu rkstu Mar 5, 2024

Choose a reason for hiding this comment

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

Updated in dingo_walkthrough.ipynb file

README.md Outdated

**Note:** If you are using `GitHub Codespaces`. Start [here](https://docs.github.com/en/codespaces/setting-up-your-project-for-codespaces/adding-a-dev-container-configuration/setting-up-your-python-project-for-codespaces) to set the python version. Once your Python version is `3.8.x` you can start following the below instructions.

Clone dingo repository
Copy link
Contributor

Choose a reason for hiding this comment

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

I propose to remove this step, cloning a repo is a straightforward command and I think not needed in installation instructions. Also there are variations of cloning not described here (https, ssh).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed cloning

## Installation

**Note:** Python version should be 3.8.x. You can check this by running the following command in your terminal:
Copy link
Contributor

Choose a reason for hiding this comment

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

great, could you please list all the possible choices and the possible solutions/workarounds in the README.
e.g. Python 3.8.18, Poetry 1.3.2 -- OK
Python 3.10.13, Poetry 1.3.2 -- fail

BTW why the second case above fails? Is there a workaround?

README.md Outdated

To install the Python dependencies, install [Poetry](https://python-poetry.org/). Then, run
To install the Python dependencies, [Poetry](https://python-poetry.org/).
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rephrase. I propose to leave the phrasing as it was before and only change the installation command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood. I've kept the original phrasing as much as possible, except for adding the installation guide for GitHub Codespaces.

README.md Outdated
You can have a look at our [Google Colab notebook](https://colab.research.google.com/github/GeomScale/dingo/blob/develop/tutorials/dingo_tutorial.ipynb)
on how to use `dingo`.

#### In local environment
You can have a look at `dingo/tutorials/dingo_walkthrough.ipynb`. Now you should be able to run all cells.
Copy link
Contributor

Choose a reason for hiding this comment

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

I propose to minimize the review process and leave the tutorial as a separate PR. So this PR will only include the changes to README. Do you agree?

README.md Outdated
You can have a look at our [Google Colab notebook](https://colab.research.google.com/github/GeomScale/dingo/blob/develop/tutorials/dingo_tutorial.ipynb)
on how to use `dingo`.

#### In local environment
You can have a look at `dingo/tutorials/dingo_walkthrough.ipynb`. Now you should be able to run all cells.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also I am not convinced that we need a "local" version of the tutorial. Isn't the current tutorial enough to execute locally?

Copy link
Contributor

@vfisikop vfisikop left a comment

Choose a reason for hiding this comment

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

Thanks.

@vfisikop vfisikop merged commit 3295932 into GeomScale:develop Mar 28, 2024
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.

None yet

2 participants