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

ENH: Make timezonefinder an optional dependency #315

Merged
merged 3 commits into from
Jan 10, 2023

Conversation

giovaniceotto
Copy link
Member

@giovaniceotto giovaniceotto commented Dec 31, 2022

Pull request type

Please check the type of change your PR introduces:

  • Code base additions (bugfix, features)

Pull request checklist

Please check if your PR fulfills the following requirements, depending on the type of PR:

  • Code base additions (for bug fixes / features):

    • Tests for the changes have been added
    • Docs have been reviewed and added / updated if needed
    • Lint (black rocketpy) has passed locally and any fixes were made
    • All tests (pytest --runslow) have passed locally

What is the current behavior?

  1. RocketPy fails to install when using Python 3.7 on Windows, since the timezonefinder package does not support Python 3.7 anymore. See Significant Dependency Issue: Timezonefinder does not support Python 3.7 #314.
  2. The timezonefinder package is rarely used and not required for any major RocketPy functionality. It is currently a required package inspite of this. And to make things worse, it requires a download larger than 40 MB.

What is the new behavior?

The timezonefinder is made an optional requirement. By default, when installing RocketPy, it will not be installed and thus RocketPy will remain compatible with Python 3.7. The total downloads needed to install RocketPy will be reduced by about 40 MB. If the user desires, he/she can install the optional requirement using:

pip install rocketpy[timezonefinder]

Note, however, that this only works with Python 3.8+.

The only feature that is not supported when timezonefinder is not installed is automatic timezone detection based on latitude and longitude when initializing the EnvironmentAnalysis class. The user needs to specify the timezone to be used for reporting. If the user does not specify it, the following error will be raised:

ImportError: The timezonefinder package is required to automatically determine local timezone based on lat,lon coordinates. Please specify the desired timezone using the `timezone` argument when initializing the EnvironmentAnalysis class or install it with `pip install timezonefinder`.

Does this introduce a breaking change?

  • Yes
  • No

Other information

This PR should solve the failing GitHub Actions that has been haunting us for a couple of months now.

@giovaniceotto giovaniceotto added the Enhancement New feature or request, including adjustments in current codes label Dec 31, 2022
@giovaniceotto giovaniceotto added this to the Release v1.0.0 milestone Dec 31, 2022
@giovaniceotto giovaniceotto self-assigned this Dec 31, 2022
@giovaniceotto giovaniceotto changed the base branch from master to develop December 31, 2022 21:06
@RocketPy-Team RocketPy-Team deleted a comment from review-notebook-app bot Dec 31, 2022
Copy link
Member

@Gui-FernandesBR Gui-FernandesBR left a comment

Choose a reason for hiding this comment

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

Everything seems to be working very well.
Questions:

  • Do you think we should keep timezonefinder in the requirement.txt file?
  • In the documentation you're modifying python version (from 3.6 to 3.7). Does this mean that we are officially dropping python 3.6 support for RocketPy? If so, should that be a big deal?

Great fix btw. In terms of coding, there's no change to ask. Only the above questions as curiosity.

@giovaniceotto
Copy link
Member Author

  • Do you think we should keep timezonefinder in the requirement.txt file?

Yes. The requirements.txt file is used for development only, it is not included in the packaged served through PyPI.

In the documentation you're modifying python version (from 3.6 to 3.7). Does this mean that we are officially dropping python 3.6 support for RocketPy? If so, should that be a big deal?

Support for Python 3.6 was dropped a while ago. Only 3.7+ is supported, that docs page was wrong. Python 3.6 stopped being supported by the Python organization in Dec 2021: https://endoflife.date/python

@giovaniceotto
Copy link
Member Author

This PR has been approved and all tests are passing, so I will merge it now.

Quick note: I have modified how RocketPy is installed by the GitHub actions before tests are performed. I switched from python setup.py install to pip install -e ., basically because pip can handle dependency version conflicts much better, and is also what users make use of.

@giovaniceotto giovaniceotto merged commit 0541011 into develop Jan 10, 2023
@Gui-FernandesBR Gui-FernandesBR deleted the enh/optional-timezonefinder branch January 10, 2023 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request, including adjustments in current codes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Significant Dependency Issue: Timezonefinder does not support Python 3.7
3 participants