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

Init repo #15

Merged
merged 21 commits into from
Sep 20, 2024
Merged

Init repo #15

merged 21 commits into from
Sep 20, 2024

Conversation

rogerkuou
Copy link
Member

Fix the following issues to setup the repository

@rogerkuou rogerkuou marked this pull request as ready for review September 16, 2024 09:31
@rogerkuou
Copy link
Member Author

rogerkuou commented Sep 16, 2024

Hi @fnattino and @vanlankveldthijs. When you have time, could you please help me review this PR?

This is a PR setting up the PyDePSI repository. I removed some old irrelevant examples. Could you please focus on:

  • See if the development guide (currently README.md of the repo) is clear and working
  • See if pyproject.toml makes sense?

The testing is failing because currently there are no tests under folder tests. They will be added in PR #9.

Thanks in advance!

Copy link
Contributor

@vanlankveldthijs vanlankveldthijs left a comment

Choose a reason for hiding this comment

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

README is clear and works perfectly.

pyproject file also looks fine to me.

The only thing I noticed is that I initially did not realize that I needed to set the python version when creating the conda environment (as described in the README).
When I then tried to pip install, it did not notify me of the requirement for python to be >=3.10 and then it did not recognize the pydepsi project and created an UNKNOWN project-0.0.0.

I don't know whether we can/should fix this. When I then did follow the instructions in the README correctly, the package installed just fine and the example notebook works.

Copy link
Contributor

@fnattino fnattino left a comment

Choose a reason for hiding this comment

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

Looks nice @rogerkuou!

The only main comment I have is regarding the dev installation procedure, but feel free to address it or discard it as you see fit.
The rest is all very minor things!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
Create a new conda environment (here we give an example name `pydepsi-dev`) with `mamba`.:

```bash
mamba create -n pydepsi-dev python=3.12
Copy link
Contributor

Choose a reason for hiding this comment

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

-c conda-forge ? Not really needed here, but I think there were some legal implications for for-profit organizations connected to the use of the anaconda channel..

README.md Outdated
Comment on lines 41 to 45
## References

- [Python packaging user guide](https://packaging.python.org/)
- [Testing in Python](https://docs.kedro.org/en/stable/development/automated_testing.html)
- [Code formatting and linting](https://docs.kedro.org/en/stable/development/linting.html)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these links provided because they are useful readings for people who starts develop the package? Maybe you could mention it here, or simply call this section "Useful reading material"?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure, are the following lines actually needed? I think that the pip build/install part already runs this step?

https://github.com/MotionbyLearning/PyDePSI/blob/cdda86dc41096ce0c64849c8cb5f70dd905a059b/.github/workflows/build.yml#L33-L34

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry I realized I forgot to push the fix of this comment. This is implemented with a separate PR #19

README.md Show resolved Hide resolved
@fnattino
Copy link
Contributor

@vanlankveldthijs

The only thing I noticed is that I initially did not realize that I needed to set the python version when creating the conda environment (as described in the README). When I then tried to pip install, it did not notify me of the requirement for python to be >=3.10 and then it did not recognize the pydepsi project and created an UNKNOWN project-0.0.0.

I was curious about what would happen in this situation as well, so I also tried to pip install the package in an environment where I had Python 3.9. But in my case pip correctly recognizes the Python version requirement in the pyproject.toml file and throw the following error:

ERROR: Ignored the following versions that require a different python version: 0.1.0 Requires-Python >=3.10; 0.1.2 Requires-Python >=3.10
ERROR: Could not find a version that satisfies the requirement sarxarray (from pydepsi) (from versions: none)
ERROR: No matching distribution found for sarxarray

So this seems to be working fine on my system. I wonder whether this is something related to the pip version (I have 24.2).

Co-authored-by: Francesco Nattino <[email protected]>
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@rogerkuou
Copy link
Member Author

Thanks @vanlankveldthijs and @fnattino for the review. I applied some changes according to @fnattino 's suggestions. And @vanlankveldthijs same as Francesco the version constraint seems working for me. Given for now the installation is mainly development oriented, I would park this for now. We can polish up the user oriented installation docs later.

Will merge this PR.

@rogerkuou rogerkuou merged commit 9f169d1 into main Sep 20, 2024
0 of 9 checks passed
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