FIX: env analysis dependency imports #403

Gui-FernandesBR · 2023-09-06T13:57:53Z

Pull request type

Please check the type of change your PR introduces:

[ x] Other (please describe): dependency management

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
- Lint (black rocketpy) has passed locally and any fixes were made
- All tests (pytest --runslow) have passed locally

What is the current behavior?

You cannot run "import rocketpy" without getting an error

What is the new behavior?

Inspired in pandas management of optional dependency, I introduced a short function inside tools.py that will deal with the lazy imports of Environment Analysis class. I hope you all like it!

Please notice that all the optional decencies related to Environment Analysis are actually not crucial to run the class.

Plus: I created the requirements-optional.txt to avoid confusions.

Does this introduce a breaking change?

No

Other information

This is supposed to be the correct version of the old PR FIX: optional imports for EnvironmentAnalysis prevent default install. #399 .
The bug was introduced with the PR MAINT: convert windrose ipywidgets and jsonpickle to optional dependencies #368

Gui-FernandesBR · 2023-09-06T14:00:24Z

This bug was reported in our discord server!

phmbressan

Really good implementation. Concise, organized and easily replicable/scalable.

Still a partial review only since I will test it further later, but things are awesome so far.

rocketpy/tools.py

rocketpy/EnvironmentAnalysis.py

Update rocketpy/tools.py Co-authored-by: Pedro Henrique Marinho Bressan <87212571+phmbressan@users.noreply.github.com>

…com/rocketpy-team/rocketpy into fix/env-analysis-dependency-imports

Gui-FernandesBR · 2023-09-06T16:40:46Z

@phmbressan I was wondering if there's any easy way to test the import rocketpy import after building the package with pip install without using the [all] option.

Basically the bug passed through our git actions because we always set it to download the package with [all] option.

Idk if this is a matter of solving it with better github actions commands or new creating unit test with pytest. But I think it may be important for us in the future

MateusStano · 2023-09-06T21:05:17Z

@phmbressan I was wondering if there's any easy way to test the import rocketpy import after building the package with pip install without using the [all] option.

You can use in Colab:

!pip install git+https://github.com/RocketPy-Team/RocketPy.git@fix/env-analysis-dependency-imports#egg=rocketpy[env_analysis]

I tested it here and it seems some warnings are raised when even when the packages are installed:

Gui-FernandesBR · 2023-09-06T21:25:02Z

@phmbressan I was wondering if there's any easy way to test the import rocketpy import after building the package with pip install without using the [all] option.

You can use in Colab:
!pip install git+https://github.com/RocketPy-Team/RocketPy.git@fix/env-analysis-dependency-imports#egg=rocketpy[env_analysis]
I tested it here and it seems some warnings are raised when even when the packages are installed:

Nice catch @MateusStano !

I solved the issue within the last commit. The code was trying to import "windrose>=1.6.8" instead of "windrose", of course it should not work properly.

I think it is running correctly now.

- no longer assigns the issue to its author (only for PRs) - deleted the useless auto-assign-projects file - runs the pytest without the [all] option

Gui-FernandesBR · 2023-09-07T14:48:12Z

@MateusStano , @phmbressan ,

This PR is ready for a re-review now.
I've to added IPython as an optional requirement for the environment analysis class.
Running the class always through jupyter gave me the false idea that IPython was built-in python module.

I hope the code will be working on your machines as well.

…git-actions

phmbressan · 2023-09-07T16:38:15Z

rocketpy/EnvironmentAnalysis.py

+        operators = [">=", "<=", "==", ">", "<"]
        for requirement in env_analysis_require:
-            try:
-                module = import_optional_dependency(requirement)
-            except ImportError:
+            pckg_name = requirement
+            for op in operators:
+                pckg_name = pckg_name.split(op)[0]
+            is_present = importlib.util.find_spec(pckg_name)
+            if is_present is None:
                print(
-                    f"{requirement:20} is not installed. Some methods may not work."
+                    f"{pckg_name:20} is not installed. Some methods may not work."
                    + " You can install it by running 'pip install rocketpy[env_analysis]'"
                )


Some questions:

Have you considered, instead of performing many loops to split the package name, using the split function of the package re native to Python? For example:

import re pckg_name = re.split("[<>=!]{1,2}", pckg_name)[0]

Your operator list lacks the !=. You could add this to the list or use suggestion I made above which eliminates the need for the operators list.

This seems to be a piece of code that might be needed in the future in many instances, I believe that it would be better to encapsulate this behavior into its own separate function in tools. For instance:

import re def check_extra_requirements(requirements): for requirement in requirements: pkg_name = re.split("[<>=!]{1,2}", requirement)[0] if importlib.util.find_spec(pkg_name) is None: ... # print error

Thanks for solid review.

I didn't know that. See in the last commit how i implemented your suggestion

gotcha!

Alright, I created a new ``tools.check_requirement_version()` function, let's see if you like it. Still prefer to keep other function in the env analysis class so we can do dedicated exception management

phmbressan · 2023-09-07T17:06:32Z

rocketpy/EnvironmentAnalysis.py

        env_analysis_require = [
            "timezonefinder",
            "windrose>=1.6.8",
+            "IPython>=8.8.0",
            "ipywidgets>=7.6.3",
            "jsonpickle",
        ]


If the package name is splitted and only the first part of the name matters for checking if it is present, why put the version numbers in this list?

I might have overlooked something, but it seems to me that

env_analysis_require = [ "timezonefinder", "windrose", "IPython", "ipywidgets", "jsonpickle", ]

would have the same effect for this function without the need of the versioning. Perhaps it would be useful to keep the versioning and also check if the installed version is compliant, it is likely that the importlib has ways to check that.

Yes I had left the numbers bc I thought in the future it would be nice to have version numbering tests.
I just "hadn't realized by my own" that this future were so close, so I implemented version checks in my last commit.
This feature can be used in other classes in the future.

Hope you like it now!

MateusStano · 2023-09-08T17:15:04Z

Still getting a warning when running on Colab:

I think maybe we should lower the IPython version required

Gui-FernandesBR · 2023-09-10T04:10:02Z

Still getting a warning when running on Colab:

I think maybe we should lower the IPython version required

Hey @MateusStano , thks for noticing that. I was wondering if there's any automatic test that we could implement to check if the simulation is running in colab environment. If weren't by you, I would never know the code isn't working there.

Anyway, I have simply dropped the version requirement of ipython. I believe there's no reason for us to demand specific version of this package.

Ready for a re-review then.

Gui-FernandesBR · 2023-09-10T04:10:35Z

Ready for re-review @phmbressan

MAINT: update github workflows files

Gui-FernandesBR · 2023-09-15T16:59:15Z

I just improved the version comparison of python packages. It seems that using eval() may not work in 100% of the cases, besides being a security risk.

https://stackoverflow.com/questions/11887762/how-do-i-compare-version-numbers-in-python

phmbressan · 2023-09-15T22:46:18Z

While testing the latest changes, I ran into this import error:

Python 3.11.5 (main, Sep  7 2023, 19:25:29) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import importlib
>>> importlib.metadata
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module 'importlib' has no attribute 'metadata'
>>>

It seems weird that the metadata is not in the standard __init__ of importlib. The GitHub tests also were passing, so to make sure the error was not due to a corrupted venv I tested with a new venv and got the same error. I fixed it with my newest commit, now it seems to be working.

phmbressan

As I mentioned I my partial review, I really enjoyed this approach to organize optional dependencies imports. Well done!

The main change this PR introduces while adding new packages is to ensure the package is also added to the optional dependency dictionary of the corresponding class exactly as it is in the setup.py.

Perhaps, in the future, it may be interesting to verify these dependencies automatically with importlib.metadata.requires('rocketpy').

Gui-FernandesBR · 2023-09-15T23:57:16Z

Thank you all for solid reviewing this PR.

I hereby confirm that the code is running locally an in Goggle Collab.

Let's move forward.

Gui-FernandesBR added 2 commits September 6, 2023 10:40

FIX: improve optional dependency imports

7bb5a09

MAINT: introducing requirements-optional.txt file

bc73fcd

Gui-FernandesBR added the Bug Something isn't working label Sep 6, 2023

Gui-FernandesBR added this to the Release v1.0.0 milestone Sep 6, 2023

Gui-FernandesBR requested review from MateusStano and phmbressan September 6, 2023 13:57

Gui-FernandesBR requested a review from giovaniceotto as a code owner September 6, 2023 13:57

Gui-FernandesBR self-assigned this Sep 6, 2023

phmbressan reviewed Sep 6, 2023

View reviewed changes

rocketpy/tools.py Outdated Show resolved Hide resolved

rocketpy/tools.py Outdated Show resolved Hide resolved

rocketpy/tools.py Outdated Show resolved Hide resolved

rocketpy/EnvironmentAnalysis.py Show resolved Hide resolved

Gui-FernandesBR and others added 3 commits September 6, 2023 13:30

MAINT: improve dependency management function

e690da6

Update rocketpy/tools.py Co-authored-by: Pedro Henrique Marinho Bressan <87212571+phmbressan@users.noreply.github.com>

FIX: applying comments reviews

7cf2435

Merge branch 'fix/env-analysis-dependency-imports' of https://github.…

4b93df6

…com/rocketpy-team/rocketpy into fix/env-analysis-dependency-imports

Gui-FernandesBR requested a review from phmbressan September 6, 2023 16:40

Gui-FernandesBR and others added 2 commits September 6, 2023 18:22

FIX: __check_extra_requirements() not working

b048e24

Fix code style issues with Black

04f6a54

Gui-FernandesBR added 3 commits September 6, 2023 22:54

MAINT: update github workflows files

174663b

- no longer assigns the issue to its author (only for PRs) - deleted the useless auto-assign-projects file - runs the pytest without the [all] option

FIX: adding IPython as an optional require

0413472

FIX: restore ipython version to 8.8 to allow py3.8

bcccebc

Gui-FernandesBR added 4 commits September 7, 2023 11:48

Merge branch 'fix/env-analysis-dependency-imports' into maint/update-…

507a5fb

…git-actions

TST: apply import_optional_dependency to tests

d032296

Merge branch 'fix/env-analysis-dependency-imports' into maint/update-…

7b72ee6

…git-actions

TST: test the import rocketpy

9ddcec9

phmbressan reviewed Sep 7, 2023

View reviewed changes

FIX: improve extra req checks

979a5c7

Gui-FernandesBR requested a review from phmbressan September 10, 2023 04:10

GIT: improve pytest workflow based on rev

5341548

Gui-FernandesBR mentioned this pull request Sep 10, 2023

MAINT: update github workflows files #405

Merged

4 tasks

Merge pull request #405 from RocketPy-Team/maint/update-git-actions

cfdd041

MAINT: update github workflows files

MateusStano approved these changes Sep 13, 2023

View reviewed changes

MAINT: avoid using eval()

6382364

FIX: importlib metadata import error.

96a8ffc

phmbressan approved these changes Sep 15, 2023

View reviewed changes

Gui-FernandesBR merged commit c0e0b1a into beta/v1.0.0 Sep 15, 2023

Gui-FernandesBR deleted the fix/env-analysis-dependency-imports branch September 16, 2023 02:38

Uh oh!

FIX: env analysis dependency imports #403

FIX: env analysis dependency imports #403

Uh oh!

Conversation

Gui-FernandesBR commented Sep 6, 2023

Pull request type

Pull request checklist

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Other information

Uh oh!

Gui-FernandesBR commented Sep 6, 2023

Uh oh!

phmbressan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Gui-FernandesBR commented Sep 6, 2023

Uh oh!

MateusStano commented Sep 6, 2023

Uh oh!

Gui-FernandesBR commented Sep 6, 2023

Uh oh!

Gui-FernandesBR commented Sep 7, 2023

Uh oh!

phmbressan Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

Gui-FernandesBR Sep 10, 2023

Choose a reason for hiding this comment

Uh oh!

phmbressan Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

Gui-FernandesBR Sep 10, 2023

Choose a reason for hiding this comment

Uh oh!

MateusStano commented Sep 8, 2023

Uh oh!

Gui-FernandesBR commented Sep 10, 2023

Uh oh!

Gui-FernandesBR commented Sep 10, 2023

Uh oh!

Gui-FernandesBR commented Sep 15, 2023

Uh oh!

phmbressan commented Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phmbressan left a comment

Choose a reason for hiding this comment

Uh oh!

Gui-FernandesBR commented Sep 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

phmbressan commented Sep 15, 2023 •

edited

Loading