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

Significant Dependency Issue: Timezonefinder does not support Python 3.7 #314

Closed
giovaniceotto opened this issue Dec 31, 2022 · 10 comments · Fixed by #315
Closed

Significant Dependency Issue: Timezonefinder does not support Python 3.7 #314

giovaniceotto opened this issue Dec 31, 2022 · 10 comments · Fixed by #315
Assignees
Labels
Bug Something isn't working

Comments

@giovaniceotto
Copy link
Member

giovaniceotto commented Dec 31, 2022

Describe the bug

Timezonefinder no longer supports python 3.7. This is a significant issue since RocketPy officially supports Python 3.7 but depends on timezonefinder.

To Reproduce

Create an environment with Python 3.7.9 on Windows and attempt to install RocketPy. The following error should occur (tested locally and via GitHub Actions):

Best match: timezonefinder 6.1.9
Processing timezonefinder-6.1.9.tar.gz
error: Couldn't find a setup script in C:\Users\ghceo\AppData\Local\Temp\easy_install-byqibg5q\timezonefinder-6.1.9.tar.gz

Expected behavior

RocketPy should install without any issues, but it fails since a required library cannot be installed.

Additional context

This started to be a problem only recently. Requiring an earlier version of timezonefinder (<6.1) would solve the issue. However, this is dangerous for many reasons since we can end up with out of dated time zone delimitations.

@giovaniceotto giovaniceotto added the Bug Something isn't working label Dec 31, 2022
@giovaniceotto giovaniceotto added this to the Release v1.0.0 milestone Dec 31, 2022
@giovaniceotto
Copy link
Member Author

giovaniceotto commented Dec 31, 2022

Did a little digging and found out that timezonefinder dropped support for version 3.7 once Numpy started supporting only versions 3.8 and above.

I believe we should do the same for release v.1.0.0: drop support for Python 3.7, support Python 3.8 - Python 3.11.
@MateusStano and @Gui-FernandesBR, do you agree?

Additional Context

Numpy dropped support for Python 3.7 since version 1.22.0, released on Dec 31, 2021. More details can be found in https://numpy.org/devdocs/release/1.22.0-notes.html and numpy/numpy#19665.

image

Official Python 3.7 end-of-life: https://endoflife.date/python

@Gui-FernandesBR
Copy link
Member

If numpy dropped 3.7, I agree dropping as well.

On the other hand, why not just dropping timezonefinder then? This is a not so light weight library, and probably there are other options available.

@Gui-FernandesBR
Copy link
Member

I think @phmbressan may have an opinion as well

@giovaniceotto
Copy link
Member Author

giovaniceotto commented Dec 31, 2022

On the other hand, why not just dropping timezonefinder then? This is a not so light weight library, and probably there are other options available.

There is no other viable alternative to timezonefinder available, in my opinion. If we drop it, we will drop the feature that automatically detects the timezone based on latitude and longitude.

Here is a comprehensive list of what is available: https://stackoverflow.com/questions/16086962/how-to-get-a-time-zone-from-a-location-using-latitude-and-longitude-coordinates

We can make it an optional dependency.

@giovaniceotto
Copy link
Member Author

giovaniceotto commented Dec 31, 2022

To add to the discussion of dropping this library, or making it optional, it is currently not implemented in the Environment class. It is only used in the EnvrionmentAnalysis class. In fact, it is only used briefly in one method: https://github.com/RocketPy-Team/RocketPy/blob/master/rocketpy/EnvironmentAnalysis.py#L346

It can easily be made into an optional dependency!

@giovaniceotto
Copy link
Member Author

I went ahead and created #315. It does not solve the discussion of dropping support for Python 3.7 or not. It only makes the timezonefinder package requirement optional, which, in my honest opinion, should be the case regardless since it is not necessary to run the basic functionality of RocketPy.

@Gui-FernandesBR Gui-FernandesBR linked a pull request Jan 1, 2023 that will close this issue
7 tasks
@MateusStano
Copy link
Member

I believe we should do the same for release v.1.0.0: drop support for Python 3.7, support Python 3.8 - Python 3.11.

It definitely makes sense to drop Python 3.7 for v1.0

Are there any other libraries that should be made optional? I just checked and there are a few other EnvironmentAnalysis libraries that are required for installation, i.e. windrose, jsonpickle and ipywidgets.

I'd suggest we open a PR to remove the Python 3.7 dependency and make these other libraries optional as well

@giovaniceotto
Copy link
Member Author

Matplotlib has dropped support for Python 3.7 in its latest version as well.

@Gui-FernandesBR
Copy link
Member

I believe we should do the same for release v.1.0.0: drop support for Python 3.7, support Python 3.8 - Python 3.11.

It definitely makes sense to drop Python 3.7 for v1.0

Are there any other libraries that should be made optional? I just checked and there are a few other EnvironmentAnalysis libraries that are required for installation, i.e. windrose, jsonpickle and ipywidgets.

I'd suggest we open a PR to remove the Python 3.7 dependency and make these other libraries optional as well

I recently created the PRs #365 and #368, which are going to drop the support for python version 3.7 in rocketpy and reduce the number of mandatory dependencies, respectively.

After merging these two PRs, I believe we are going to be able to close this one, finally.

@MateusStano
Copy link
Member

Solved in #365 and #368

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants