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

Fix setup.py and export public functions #7

Merged
merged 6 commits into from
Jul 30, 2024
Merged

Conversation

Ianlmgoddard
Copy link
Contributor

This PR makes changes to allow a user to import this package from another environment. The following changes are made:

  • Modify setup.py to first allow the package smart_building_rating_calculator to be found when installing the package, and secondly adds install_requires so the dependencies of this package are installed when this package is installed.
  • exports the following to the public API of this package.
__all__ = [
    "sbr_score",
    "BatterySize",
    "EVChargerPower",
    "HeatingSource",
    "HotWaterSource",
    "SolarInverterSize",
]
  • fixes some relative import paths.

With these changes we can now use the package in the smart-building-rating-api

@Ianlmgoddard Ianlmgoddard marked this pull request as ready for review July 29, 2024 15:43
setup.py Outdated Show resolved Hide resolved
Copy link
Member

@shengy90 shengy90 left a comment

Choose a reason for hiding this comment

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

You also need to make the project locally installable for this to work. See:

https://github.com/OpenSynth-energy/OpenSynth/blob/aa4da4a2de0abc23d7f9b09b46719ace138679e8/Pipfile#L7

@Ianlmgoddard
Copy link
Contributor Author

You also need to make the project locally installable for this to work. See:

https://github.com/OpenSynth-energy/OpenSynth/blob/aa4da4a2de0abc23d7f9b09b46719ace138679e8/Pipfile#L7

Interesting, why does this need to be added? I don't seem to need this to make it work locally, so just wondering what I'm missing here/what this step is adding?

@Ianlmgoddard Ianlmgoddard requested a review from shengy90 July 29, 2024 16:13
@shengy90
Copy link
Member

shengy90 commented Jul 29, 2024

Interesting, why does this need to be added? I don't seem to need this to make it work locally, so just wondering what I'm missing here/what this step is adding?

This makes sures that the your project is installed as per specified in your setup.py, and pipenv will make sure that your piplock file is compatible with what your setup.py as asking, to ensure consistency in your environments.

There are 2 types of users of your packages:

  1. End users, who will be doing pipenv install your-package from Pypi. That file will be built as according to your setup.py. Completely independent of your pipfile/ piplock or any other virtual environment manager you're using (e.g. poetry)
  2. Developers/ contributors, who will be cloning your repo and doing pipenv sync --dev which installs from your Pipfile.lock.

If you don't specify your local project as your dependency, your environment won't be consistent. For example in your setup.py you have install_requires = [pandas >= 2.2.2]. But locally you're using pandas==2.0.0, and specifically a version of pandas that works in 2.0.0 but no longer in 2.2.2.

If you do not list your project as a dependency in your Pipfile, Pipenv will not resolve this discrepancy, i.e.:

  • End users will be installing Pipenv = 2.2.2.
  • Other developers when they clone your repo and did pipenv sync --dev, they are installing from your Piplock.file (pandas=2.0.0) and develop with something that works on 2.0.0 but no longer on 2.2.2 what your end users have installed.

If you list your project as dependency in your Pipfile, Pipenv will resolve this discrepancy and automatically upgrade your Pipfile.lock to 2.2.2 (as specified in your setup.py). That way you can ensure that your Pipfile.lock is consistent with your setup.py.

This example is very simple as we're only talking about 1 dependency (Pandas). But in a large project, dependencies can very quickly become messy and you can easily end up in a situation where your setup.py isn't consistent with your Piplock.file. Making your project its own dependencies will resolve that.

Pipenv official docs:

README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@charlotte-avery charlotte-avery left a comment

Choose a reason for hiding this comment

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

Update README to explain what the package is used for

Copy link
Contributor

@charlotte-avery charlotte-avery left a comment

Choose a reason for hiding this comment

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

Looks great, thanks so much Ian!

Copy link
Member

@shengy90 shengy90 left a comment

Choose a reason for hiding this comment

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

LGTM. Great work!

@Ianlmgoddard Ianlmgoddard merged commit 491817a into staging Jul 30, 2024
1 check passed
@Ianlmgoddard Ianlmgoddard mentioned this pull request Jul 30, 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.

3 participants