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

feat: Differentiate packages #2390

Draft
wants to merge 11 commits into
base: develop
Choose a base branch
from
Draft

Conversation

ReimarBauer
Copy link
Member

@ReimarBauer ReimarBauer commented May 24, 2024

Purpose of PR?:
This makes it possible to install mswms without all packages needed for the whole MSS

There are some steps needed to get then to something like the splitting seen for basemap

https://github.com/matplotlib/basemap

Fixes #2307

Does this PR introduce a breaking change?
splits modules

If the changes in this PR are manually verified, list down the scenarios covered::
without pytest-qt
pytest -v -n 6 --dist loadfile --max-worker-restart 4 --durations=20 tests/_test_mswms

============================================================== slowest 20 durations ==============================================================
2.65s call     tests/_test_mswms/test_mss_plot_driver.py::Test_VSec::test_repeated_locations
2.55s call     tests/_test_mswms/test_mss_plot_driver.py::Test_HSec::test_HS_CloudsStyle_01
2.33s call     tests/_test_mswms/test_mss_plot_driver.py::Test_VSec::test_VS_GenericStyle
2.29s call     tests/_test_mswms/test_mss_plot_driver.py::Test_HSec::test_degree_crs_codes[EPSG:77890010]
2.15s call     tests/_test_mswms/test_wms.py::Test_WMS::test_produce_vsec_plot
1.99s call     tests/_test_mswms/test_mss_plot_driver.py::Test_HSec::test_meter_crs_codes[EPSG:3413]
1.98s call     tests/_test_mswms/test_mss_plot_driver.py::Test_HSec::test_meter_crs_codes[EPSG:3995]
1.85s call     tests/_test_mswms/test_mss_plot_driver.py::Test_HSec::test_HS_GenericStyle_other
1.82s call     tests/_test_mswms/test_mss_plot_driver.py::Test_HSec::test_degree_crs_codes[MSS:lcc,20,0,40,20]
1.78s call     tests/_test_mswms/test_mss_plot_driver.py::Test_HSec::test_degree_crs_codes[MSS:stere,20,40,40]
1.73s call     tests/_test_mswms/test_wms.py::Test_WMS::test_produce_hsec_plot
1.68s call     tests/_test_mswms/test_mss_plot_driver.py::Test_HSec::test_degree_crs_codes[MSS:cass,20,40]
1.68s call     tests/_test_mswms/test_mss_plot_driver.py::Test_HSec::test_degree_crs_codes[EPSG:77790010]
1.61s call     tests/_test_mswms/test_mss_plot_driver.py::Test_HSec::test_HS_WStyle_PL_01
1.60s call     tests/_test_mswms/test_mss_plot_driver.py::Test_HSec::test_HS_GeopotentialWindStyle_PL
1.41s call     tests/_test_mswms/test_mss_plot_driver.py::Test_VSec::test_VS_TemperatureStyle_01
1.37s call     tests/_test_mswms/test_mss_plot_driver.py::Test_VSec::test_VS_RelativeHumdityStyle_01
1.32s call     tests/_test_mswms/test_mss_plot_driver.py::Test_VSec::test_VS_HorizontalVelocityStyle_01
1.31s call     tests/_test_mswms/test_mss_plot_driver.py::Test_VSec::test_VS_ProbabilityOfWCBStyle_01
1.30s call     tests/_test_mswms/test_mss_plot_driver.py::Test_HSec::test_meter_crs_codes[EPSG:3031]
============================================ 113 passed, 3 skipped, 25 warnings in 102.75s (0:01:42) =============================================

Additional information for reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs

Does this PR results in some Documentation changes?
If yes, include the list of Documentation changes

Checklist:

@ReimarBauer
Copy link
Member Author

ReimarBauer commented May 24, 2024

currently one can use the mwms.txt requirements to feed into the meta.yaml and build locally a package for MSS only with modules needed for mswms

that jinja2 idea sounds interesting, to have one template for all options until we have subpackages on conda-forge
https://stackoverflow.com/questions/61485382/add-requirements-from-requirements-txt-to-conda-meta-yaml

For testings we need a way to base the test environment on the provided dependencies. And the development.txt for testing has for the mswms server too much packages at least pytest-qt needs to be optional.

I don't know the condition for setup.py to exclude not needed console_scripts.

@ReimarBauer ReimarBauer marked this pull request as draft May 24, 2024 07:10
@ReimarBauer
Copy link
Member Author

@matrss if we revise the conftest, we should see that we distribute the functionalities and also the fixtures.

@matrss
Copy link
Collaborator

matrss commented May 24, 2024

I am pretty sure that all of those try: ... except ModuleNotFoundError: ... blocks will break the test suite in many ways. I'd rather not have those and make it clear that the test suite can only run with everything there. There are many tests that break the boundaries between these packages (e.g. msui tests that access the mscolab database). When splitting into proper packages we could extract the tests that really only apply to that package into their own tests/ directory (that of the new package) and re-use those again in the parts of the test suite that test all three together.

Also, I don't see how this splits of mswms into its own package that would be independently installable, yet.

@ReimarBauer
Copy link
Member Author

ReimarBauer commented May 24, 2024

I am pretty sure that all of those try: ... except ModuleNotFoundError: ... blocks will break the test suite in many ways.

all tests succeeds.

I'd rather not have those and make it clear that the test suite can only run with everything there.

yes, I think we need later more than one conftest.py if we want to test such a sub package.

There are many tests that break the boundaries between these packages (e.g. msui tests that access the mscolab database). When splitting into proper packages we could extract the tests that really only apply to that package into their own tests/ directory (that of the new package) and re-use those again in the parts of the test suite that test all three together.

Also, I don't see how this splits of mswms into its own package that would be independently installable, yet.

I solved this locally by an own conda build . of the recipe with the limitation of the requirements and edited setup.py. next I look on mscolab what from the library needs to become separated module wise. When there is an other localbuild/meta.yaml possible these scripts can become organized.

@matrss
Copy link
Collaborator

matrss commented May 24, 2024

all tests succeeds.

In CI yes, because all dependencies are installed. There these try blocks serve no purpose. If I just uninstall keyring and run the test suite I get a bunch of errors already. If I uninstall PyQt5 it will be a lot more.

yes, I think we need later more than one conftest.py if we want to test such a sub package.

Not later, that is a necessary part of the split, unless we want to split into individually-untested sub-packages (which could be fine, but then again, the test suite as is requires all of those dependencies to function and the try blocks to make them optional break a lot of stuff).

I solved this locally by an own conda build . of the recipe with the limitation of the requirements and edited setup.py. next I look on mscolab what from the library needs to become separated module wise. When there is an other localbuild/meta.yaml possible these scripts can become organized.

They need to become individual python packages, with their own pyproject.toml/setup.cfg/setup.py or however else you want to create those. Otherwise they aren't separate packages.

@ReimarBauer
Copy link
Member Author

ReimarBauer commented May 24, 2024

In CI yes, because all dependencies are installed. There these try blocks serve no purpose. If I just uninstall keyring and run the test suite I get a bunch of errors already. If I uninstall PyQt5 it will be a lot more.

This is for me currently only an intermediate step. I want to figure if when I use mswms.txt as a base of the sub package, if the mswms tests succeeds and I principle have all needed packages. Or when I reduce them the test still works.

Besides the mssdev with all packages I have additional an mswmsdev with only these packages.

@matrss
Copy link
Collaborator

matrss commented May 24, 2024

I just commented on this because you mentioned me. I see that this is a draft, so as long as those try-blocks are not intended to be in the to-be-merged version of this PR that is fine by me, I don't really care how you do the exploratory stuff of figuring out what to do. I just wanted to say that I would not approve these try-blocks as a change, since they wouldn't make any sense on stable or develop.

@ReimarBauer
Copy link
Member Author

we can keep the setup.py as is and use a patch in the build process on conda-forge to remove the unrelated setup entries.

@ReimarBauer ReimarBauer mentioned this pull request May 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Find a way to split into different packages with same codebase
2 participants