search

Breakthrough-Energy / PreREISE

Generate input data for scenario framework
https://breakthrough-energy.github.io/docs/
MIT License
20 stars 28 forks source link

refactor: change pygrib to a lazy import #280

Closed danielolsen closed 2 years ago

danielolsen commented 2 years ago

Pull Request doc

Purpose

Avoid importing pygrib when it's not necessary. This package is hard or impossible to install in a Windows environment, due to a dependence on compiled ECCODES code, so delaying the import should prevent problems when trying to use unrelated parts of the package.

What the code is doing

Within calculations.py: moving the pygrib import within the functions that need it, rather than at module level. There should be no change in functionality.

Within test_calculations.py: refactoring how pygrib is mocked to account for the new behavior within calculate_pout_blended.

Time estimate

5 minutes, maybe longer if someone wants to propose an alternative that achieves the same thing.

danielolsen commented 2 years ago

It looks good to me although I can't reproduce the issue on my end. Also curious about different solutions.

I bet if you uninstall pygrib, you could test that develop doesn't work, but that this branch would (at least, the tests would pass).

rouille commented 2 years ago

pip installing pygrib is not an issue? Just importing?

danielolsen commented 2 years ago

pip installing pygrib is not an issue? Just importing?

The installing is the root of the issue. It doesn't seem to be able to install without the compiled binaries, which don't seem to be available for Windows.

rouille commented 2 years ago

pip installing pygrib is not an issue? Just importing?

The installing is the root of the issue. It doesn't seem to be able to install without the compiled binaries, which don't seem to be available for Windows.

Then do we need to do something for the requirements/Pipfile files?

danielolsen commented 2 years ago

pip installing pygrib is not an issue? Just importing?

The installing is the root of the issue. It doesn't seem to be able to install without the compiled binaries, which don't seem to be available for Windows.

Then do we need to do something for the requirements/Pipfile files?

Maybe. pygrib isn't really a dev package, but moving it to that section would improve the package usability for Windows users while degrading the package usability for Unix-based users who want to generate wind profiles out of the box. Not sure what the right move is here. If we still had an old-style setup.py script, we could change the installation based on checking the user's OS, but I'm not sure how to build that into the Pipfile-based workflow (or if we even want to).

rouille commented 2 years ago

pip installing pygrib is not an issue? Just importing?

The installing is the root of the issue. It doesn't seem to be able to install without the compiled binaries, which don't seem to be available for Windows.

Then do we need to do something for the requirements/Pipfile files?

Maybe. pygrib isn't really a dev package, but moving it to that section would improve the package usability for Windows users while degrading the package usability for Unix-based users who want to generate wind profiles out of the box. Not sure what the right move is here. If we still had an old-style setup.py script, we could change the installation based on checking the user's OS, but I'm not sure how to build that into the Pipfile-based workflow (or if we even want to).

I am wondering if we should have different installation for different project in PreREISE. A user might be interested in generating solar profiles only and does not need all the packages. I am wondering if it is possible to segment the requirements/Pipfile with markers as we do for the tests

rouille commented 2 years ago

pip installing pygrib is not an issue? Just importing?

The installing is the root of the issue. It doesn't seem to be able to install without the compiled binaries, which don't seem to be available for Windows.

Then do we need to do something for the requirements/Pipfile files?

Maybe. pygrib isn't really a dev package, but moving it to that section would improve the package usability for Windows users while degrading the package usability for Unix-based users who want to generate wind profiles out of the box. Not sure what the right move is here. If we still had an old-style setup.py script, we could change the installation based on checking the user's OS, but I'm not sure how to build that into the Pipfile-based workflow (or if we even want to).

I am wondering if we should have different installation for different project in PreREISE. A user might be interested in generating solar profiles only and does not need all the packages. I am wondering if it is possible to segment the requirements/Pipfile with markers as we do for the tests

You can do platform specific installation with pipenv: https://pipenv-fork.readthedocs.io/en/latest/advanced.html#specifying-basically-anything. @jon-hagg, what is the best way to move forward with the pygrib dependency?

jenhagg commented 2 years ago

We could use this if we want to keep it by default for non windows users:

pygrib = {version = "~=2.1.4", sys_platform = "!= 'win32'"}
danielolsen commented 2 years ago

We could use this if we want to keep it by default for non windows users:

pygrib = {version = "~=2.1.4", sys_platform = "!= 'win32'"}

That seems to do it! I can now natively pipenv install, which also generates a new Pipfile.lock along the way (printing Ignoring pygrib: markers 'sys_platform != "win32"' don't match your environment in the process), and using the new Pipfile.lock I can pipenv sync.