Open-EO / openeo-gfmap

Generic framework for EO mapping applications building on openEO
Apache License 2.0
4 stars 0 forks source link

runtime dependencies: pytest-dependency loguru cftime? #14

Closed soxofaan closed 4 months ago

soxofaan commented 5 months ago

https://github.com/Open-EO/openeo-gfmap/blob/c69f60a70c56d43fb1d1843198061b042a9c7bd2/pyproject.toml#L41

when updating my local venv, I noticed this pytest-dependency package dependency.

It's listed as runtime dep, which is strange. I guess this was only intended as a dev/test depedency.

Moreover, I don't see it being used in the tests either, so I wonder if this can just be removed

soxofaan commented 5 months ago

Some other runtime dependencies I see are cftime and loguru which are also not used as far as I see.

I would recommend to keep the number of (runtime) dependencies to an essential minimum, and be very thoughtful when adding more because of long term costs/risks of third party dependencies .

GriffinBabe commented 5 months ago

@soxofaan pytest-dependency is used in the following test https://github.com/Open-EO/openeo-gfmap/blob/main/tests/test_openeo_gfmap/test_s2_fetchers.py#L290 - It requires to be executed after two previous tests as it is comparing the output of two different backends

cftime is a necessary package to read netcdf files outputted by Open-EO, and loguru is not used yet but will most likely be in the future. That said we can temporarily move them in the [dev] dependencies.

soxofaan commented 5 months ago

But pytest-depdendency is a dev or test dependency then? It should not be listed under the runtime deps

soxofaan commented 5 months ago

loguru is not used yet but will most likely be in the future.

Is there an important reason to work with loguru? While I haven't used it myself, I understand that it indeed simplifies logging and configuration. However note that most packages, like openeo, just depends on stdlib logging, if you add loguru as a requirement, you force your end user to configure two logging packages. I think it makes sense to use loguru in final application code, but it does not belong in libraries like openeo or gfmap in my opintion.

soxofaan commented 5 months ago

cftime is a necessary package to read netcdf files outputted by Open-EO

can you give more details here? I haven't heard of that package before

soxofaan commented 5 months ago

pytest-dependency is used in the following test https://github.com/Open-EO/openeo-gfmap/blob/main/tests/test_openeo_gfmap/test_s2_fetchers.py#L290 - It requires to be executed after two previous tests as it is comparing the output of two different backends

that pytest.mark.depends(on=["test_sentinel2_l2a"]) usage is apparently from the pytest-depends package, not pytest-dependency

soxofaan commented 5 months ago

Already fixed the pytest-depends/pytest-dependency confusion in b680aa6

soxofaan commented 5 months ago

also moved pytest-depends to test dependencies in 3cc7d55

soxofaan commented 5 months ago

still to figure out in this ticket

GriffinBabe commented 4 months ago

Let's remove loguru and leave it to logging then. Should we create a single logger for the entire project, or already start figuring out how we organize it in sub loggers?

CFTIME is often used in NetCDF files for the coordinates within the temporal dimension. I do not remember an example right now, but it happened me to be stucked quite a few times when not having this library installed. It's pretty lightweight also https://unidata.github.io/cftime/

soxofaan commented 4 months ago

Let's remove loguru and leave it to logging then. Should we create a single logger for the entire project, or already start figuring out how we organize it in sub loggers?

common practice is just to put something like

_log = logging.getLogger(__name__)

at the top of each module where you need a logger and use that _log logger. This patterns automatically takes care of organizing sub-loggers, keeping it simple for developer and still allows fine-grained control for end user if they want that

soxofaan commented 4 months ago

I don't mind having cftime as dependency if that is important. I'm just curious what it helps with