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

Add pyproject toml #52

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

rdmolony
Copy link

@rdmolony rdmolony commented Dec 17, 2020

I've added a pyproject.toml (via poetry) to manage dependencies and make this repo pip installable.

To manage project dependencies and to make it pip installable
Now that rc_buildingsimulator is a library can
replace all relative imports with absolute imports
and delete add to path sections of code
@rdmolony rdmolony closed this Dec 17, 2020
@rdmolony rdmolony reopened this Dec 17, 2020
Undo changes to imports etc so that CI passes
* Add pyproject.toml

To manage project dependencies and to make it pip installable

* Rename imports in each module

Now that rc_buildingsimulator is a library can
replace all relative imports with absolute imports
and delete add to path sections of code

* Revert rc_buildingsimulator to rc_simulator

Undo changes to imports etc so that CI passes
So that building_physics can be imported via rc_buildingsimulator
- see notebooks/10-min-guide...
So installs a dev version of rc_buildingsimulator in CI
and resolve CI bug - No module named 'rc_buildingsimulator'
poetry run flake8 resulted in ...
ModuleNotFoundError: No module named 'importlib_metadata'
for Python 3.6 and 3.7 ...
python-poetry/poetry#1487
Copy link
Collaborator

@pjayathissa pjayathissa left a comment

Choose a reason for hiding this comment

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

Firstly, I love the PR, thank you so much. This is my first time using poetry, so pardon my ignorance. I have a set of questions before I can approve this.

  1. Many users of this tool just clone the repo, globally pip install the dependencies and code away. They are often scientists and researchers with basic coding skills. With vagrant, I was able to offer both options. I could allow people to use vagrant to have a dependency manager, but if they didn't want that, they could just globally install the dependencies (most people have pandas, numpy etc already installed anyway). My concern with poetry is that it is a requirement and only runs through poetry. This has two issues
    a. The user has to install poetry (following the instructions on the poetry website didn't work for me as (even with sudo) poetry can't manage my .bash_profile. I therefore had to homebrew it). This can be a bit overwhelming for the person who is just using a gui python installation like anaconda.
    b. We become dependent on poetry

It would be great to somehow create this inbetween situation. Perhaps for now we could have two branches. The main, and dist? Once our user base is happy with the pypi and it's had some user testing, we can merge this dist branch to main

  1. In order to push this to pypi, do I just run poetry build, poetry publish? Is there anything else I need to watch out for
  2. Is there a reason that you changed the name from rc_simulator to rc_buildingsimulator. The latter seems quite verbose
  3. I had issues running tests/testExamples.py. FileNotFoundError: [Errno 2] No such file or directory: 'examples/annualSimulation.py'

Looking forward to your reply. And sorry for the massive delay in reviewing. I work in healthcare at the moment, and the pandemic makes things quite busy.

pyproject.toml Outdated Show resolved Hide resolved
@@ -11,7 +11,7 @@

::

from buildingPhysics import Zone #Importing Zone Class
from rc_buildingsimulator.building_physics import Zone #Importing Zone Class
Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of curiosity. I there a reason for changing the headings from rc_simulator to rc_buildingsimulator?

@@ -75,8 +75,8 @@

"""

import supply_system
import emission_system
from rc_buildingsimulator import supply_system
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have a potential issue here. A lot of people that use this tool have basic python skills (they are often scientists and researchers, not developers). Package managing with poetry is nice, but this means that people will need to install poetry in order to run the script. Currently, I have used vagrant as a local dependency manager, and lazy python programmers can just skip the vagrant step by globally installing the necessary packages and getting on with it. I can't quite seem to do this with poetry and am hesitant as there is a risk that it will form a limitation to uptake. Do you have any ideas of how we can get around this?

As I understand, once we get this on pypi, people will just pip install it anyway

@rdmolony
Copy link
Author

rdmolony commented Mar 7, 2021

Hi @pjayathissa, thanks for the feedback and no worries on the delay!

  1. As of v10.0 pip supports installing directly from a pyproject.toml file via pip install . so unless users are changing the source code they won't need it. If they are; the latest pip v21.0.1 requires a setup.py for pip install -e . editable installs so they'll need to use poetry install instead. I'm not familiar with vagrant as I tend to use docker or conda for environment management - conda install -c conda-forge poetry might resolve the homebrew issue as it comes included with Anaconda?
  2. The dist and main branch seems like a handy way to just use poetry to publish to pypi if you don't want to pull it into the pyproject.toml into the main branch just yet & poetry build + poetry publish is all I do anyway!
  3. Probably to bring it in sync with the name of the repo - makes no difference to me!
  4. I'll look into it! Weird that it wasn't caught by CI

rdmolony added 2 commits March 7, 2021 18:19
To prevent a plot window from appearing during
unit testing
@rdmolony
Copy link
Author

rdmolony commented Mar 8, 2021

I just remembered that prior to poetry build + poetry publish you'll need to poetry version <CMD> as in the docs which updates the pyproject.toml version to the current version

@pjayathissa
Copy link
Collaborator

Thanks for the quick reply. You don't need to worry about running testExamples through CI. I just have that as a personal thing on my laptop to make sure that the examples are not failing.

1-2. I am leaning towards the idea of running this as a seperate dist branch, until the userbase has comfortably transitioned from using the git code-base to pip. There are a few PhD thesis that hang on this software so I don't want to make their life suddenly more complicated. Then once the dust has settled, we can merge to master. The changes to the base code is relatively minor so hopefully, this will work without too much drama.
3. Syncing with the repo name is a good idea. I'm just torn between the long name, and the reality that "RC Simulator" actually translates to "radio control simulator", or actual electrical RC circuits. I guess we keep it as it is.
4. Cool thanks, and no need to integrate it into CI. I had some issues with the matplotlib in the past

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.

2 participants