Giter VIP home page Giter VIP logo

Comments (19)

KeithB avatar KeithB commented on June 9, 2024 4

To be honest, my preference would still be a small change to the existing 3.x.x PyOWM allowing config to access OneCall3 APIs but default behaviour to OneCall1. That is fairly straight forward and gets the likes of HA working again.

I don't disagree on the potential for more change in OneCall3 (particularly given the push on their site and the other money making APIs are all V3) but without any inside information on what or when, why do a re-write/clone (comparing the OneCall API and PyOWM code it looks to be over 90% aligned) if not needed? From a library usage perspective I'd also argue that sticking with the weather manager simplifies continued use of the API and the user not having to worry about quirks/differences between Weather and OneCall APIs for now.

Happy to follow the lead of those with closer ties to PyOWM that know better than me but I'd argue for non-breaking change (i.e. config and defaults) that enables OneCall3 in PyOWM 3.x.x and kicking off 4.x.x with the introduction of new fields on top of that with the addition of national weather alerts. (I still don't think there is enough difference to warrant a onecall_manager rather than extension of weather_manager even then.) This also gets HA et al working again leaving time to debate re-structures for new features.

from pyowm.

KeithB avatar KeithB commented on June 9, 2024 1

Potential fix at https://github.com/KeithB/pyowm-onecall3/tree/dev.

  • Splits onecall API to 3.0 but leaves rest of weather on 2.5
  • Updates onecall history based on openweathermap changes where free tier onecall only allows single hour of data. (Other historical is now with chargable history API still on 2.5)
  • Daily temperature readings may have different indexes (min rather than temp_min) - can't find old openweathermap documentation to compare.

Has been tested to prove connectivity with a local stub but not put through the standard pyowm test framework.

from pyowm.

jafoobari avatar jafoobari commented on June 9, 2024

@KeithB I took a crack at running the pyowm test framework against the dev branch of your fork, and I'm hitting an error around one of your code changes when running the unit tests:

======================================================================
ERROR: test_one_call_from_dict_current_missing (tests.unit.weatherapi25.test_one_call.TestOneCall)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/bari/pyowm-onecall3/tests/unit/weatherapi25/test_one_call.py", line 99, in test_one_call_from_dict_current_missing
    self.assertRaises(ParseAPIResponseError, lambda: OneCall.from_dict(data))
  File "/Users/bari/.asdf/installs/python/3.10.6/lib/python3.10/unittest/case.py", line 738, in assertRaises
    return context.handle('assertRaises', args, kwargs)
  File "/Users/bari/.asdf/installs/python/3.10.6/lib/python3.10/unittest/case.py", line 201, in handle
    callable_obj(*args, **kwargs)
  File "/Users/bari/pyowm-onecall3/tests/unit/weatherapi25/test_one_call.py", line 99, in <lambda>
    self.assertRaises(ParseAPIResponseError, lambda: OneCall.from_dict(data))
  File "/Users/bari/pyowm-onecall3/pyowm/weatherapi25/one_call.py", line 90, in from_dict
    raise exceptions.APIResponseError("OWM API: no current or historical data returned.")
pyowm.commons.exceptions.APIResponseError: OWM API: no current or historical data returned.

The test is hitting the error here on line 17 of /tests/unit/weatherapi25/test_one_call.py.

Which is calling your code change on line 90 of pyowm/weatherapi25/one_call.py.

I tried troubleshooting it but I don't really know what's happening...I had enough trouble getting tox to play nice and run (and it's still only partially cooperating) 😅 .

I'm happy to help however I can with getting these changes fully tested and in a pull request though. Those of us using the OWM Home Assistant integration are reliant on pyowm.

from pyowm.

KeithB avatar KeithB commented on June 9, 2024

@jallen92 Thanks for that - I ran out of time today to do much more (though I'm old school and prefer dev vs test separation - well volunteered!). It's probably going to be Monday before I can take a proper look at this.

The error suggests it can't read current data from OneCall, but I have a test stub that's happy reading current data direct from OpenWeatherMap vs a test tool that's using hard coded test data (test_one_call.py).

The line in question that's failing is deliberately introducing the failure scenario as pyowm (currently) insists on current data being returned (it fails object instantiation without it) but (setting aside HA for a minute despite that being the reason I'm here!) the openweathermap API actually allows current data to be excluded from the response. Basically I took enforcing existing pyowm behaviour (mirrored in that test data from what I can tell) for stability of existing users over openweathermap optionality.

from pyowm.

KeithB avatar KeithB commented on June 9, 2024

@jallen92 What problems did you have setting tox up? I had to amend tox.ini to reflect python 3.9/3.10 but other than that, only errors I got were related to either APIs I don't pay for or non-onecall APIs still on 2.5 I don't have access to:
======================================================================== short test summary info ======================================================================== FAILED agroapi10/test_integration_polygons_api_subset.py::IntegrationTestsPolygonsAPISubset::test_polygons_CRUD - pyowm.commons.exceptions.UnauthorizedError: Invalid ... FAILED agroapi10/test_integration_soil_data.py::IntegrationTestsSoilData::test_call_soil_data - pyowm.commons.exceptions.UnauthorizedError: Invalid API Key provided FAILED airpollutionapi30/test_integration_pollutionapi30.py::IntegrationTestsPollutionAPI30::test_air_quality_at_coords - AttributeError: 'list' object has no attribu... FAILED airpollutionapi30/test_integration_pollutionapi30.py::IntegrationTestsPollutionAPI30::test_air_quality_forecast_at_coords - AssertionError: [] is not true FAILED alertapi30/test_integration_alertapi30.py::IntegrationTestsAlertAPI30::test_triggers_CRUD - pyowm.commons.exceptions.TimeoutError: API call timeouted FAILED geocodingapi10/test_integration_geocodingapi10.py::IntegrationTestsGeocodingAPI::test_reverse_geocode - AssertionError: False is not true FAILED tiles/test_integration_tile_manager.py::TesIntegrationTileManager::test_tiles_fetch - pyowm.commons.exceptions.NotFoundError: Unable to find the resource FAILED weatherapi25/test_integration.py::IntegrationTestsWebAPI25::test_forecast_at_coords_daily - pyowm.commons.exceptions.UnauthorizedError: Invalid API Key provided FAILED weatherapi25/test_integration.py::IntegrationTestsWebAPI25::test_forecast_at_id_daily - pyowm.commons.exceptions.UnauthorizedError: Invalid API Key provided ERROR agroapi10/test_integration_satellite_imagery_download.py::IntegrationTestsSatelliteImageryDownload::test_download_geotiff - pyowm.commons.exceptions.Unauthorize... ERROR agroapi10/test_integration_satellite_imagery_download.py::IntegrationTestsSatelliteImageryDownload::test_download_png - pyowm.commons.exceptions.UnauthorizedErr... ERROR agroapi10/test_integration_satellite_imagery_download.py::IntegrationTestsSatelliteImageryDownload::test_download_tile - pyowm.commons.exceptions.UnauthorizedEr... ERROR agroapi10/test_integration_satellite_imagery_download.py::IntegrationTestsSatelliteImageryDownload::test_persisting_to_disk - pyowm.commons.exceptions.Unauthori... ERROR agroapi10/test_integration_satellite_imagery_search.py::IntegrationTestsSatelliteImagerySearch::test_detailed_search - pyowm.commons.exceptions.UnauthorizedErro... ERROR agroapi10/test_integration_satellite_imagery_search.py::IntegrationTestsSatelliteImagerySearch::test_search_all - pyowm.commons.exceptions.UnauthorizedError: In... ERROR agroapi10/test_integration_satellite_imagery_search.py::IntegrationTestsSatelliteImagerySearch::test_search_for_falsecolor_png_only - pyowm.commons.exceptions.U... ERROR agroapi10/test_integration_satellite_imagery_search.py::IntegrationTestsSatelliteImagerySearch::test_search_for_geotiff_type_only - pyowm.commons.exceptions.Una... ERROR agroapi10/test_integration_satellite_imagery_search.py::IntegrationTestsSatelliteImagerySearch::test_search_for_ndvi_preset_only - pyowm.commons.exceptions.Unau... ERROR agroapi10/test_integration_satellite_imagery_search.py::IntegrationTestsSatelliteImagerySearch::test_search_for_one_satellite - pyowm.commons.exceptions.Unautho... ERROR agroapi10/test_integration_satellite_imagery_stats.py::IntegrationTestsSatelliteImageryStats::test_stats_for_satellite_image - pyowm.commons.exceptions.Unauthor... =============================================================== 9 failed, 47 passed, 11 errors in 35.33s ================================================================

from pyowm.

jafoobari avatar jafoobari commented on June 9, 2024

@KeithB the problems I ran into with setting tox up were around the specific python executable that tox was using and it not having access to a package it needed (geojson). I ended up solving it the hard way by just installing the packages in requirements.txt via the python 3.10 executable tox is using. But I could've just made my life easy and ran the tests with my own python executable and environment by running pythonX.Y setup.py test -s tests.unit manually.

And ah that makes sense regarding the errors/failures for endpoints requiring paid access. Based on what you're saying then it sounds like your changes are good to formally introduce as a pull request to the main repo!

from pyowm.

KeithB avatar KeithB commented on June 9, 2024

@jallen92 Not quite yet its not. There's a mapping issue for daily data that needs a change to avoid breaking existing users (onecall API will result in min_temp being labeled min in the object because of the way the weather parser works).

I'd also like to see the if I can get access to the 2 APIs from the weather module that failed with authorisation failures - more to find/fix anything that could break users as I've adjusted the main weather parser object.

Did you get a successful run of Tox? Always prefer a test sign off if it's possible!

from pyowm.

jafoobari avatar jafoobari commented on June 9, 2024

Gotcha!

And still no luck with getting a full successful run with Tox. I may have time later today or tomorrow to try again.

from pyowm.

KeithB avatar KeithB commented on June 9, 2024

@csparpa Couple of questions on pyowm preferences/approaches - both of them basically boil down to whether this fix would be presented as "don't use if not using OneCall3" or not.

  1. The daily mapping issue I mention above seems to already be baked in to behaviours with test data showing that a temperature entry for daily will (by code) be copied to min and max values. This contradicts structures built directly in the weather.py code where the fields are named temp_min and temp_max. i.e. The weather object will have a temperature array with different definitions depending on the API call made to OWM.
  2. I'm waiting on feedback from OWM but currently assuming that old API keys with access to the 2.5 API have not all be uplifted for access to the 3.0 API.

Is the project preference here to ensure no breaking change (i.e. existing 2.5 users can take the update with no change/impact to them) or to follow OWMs lead and assume 2.5 should be discouraged/on-going use should prefer 3.0?

from pyowm.

csparpa avatar csparpa commented on June 9, 2024

@KeithB sorry for being late at the party... the approach that has been used so far for PyOWM is to shield as much as possible users and their applications from OWM quirks and changes.
Therefore I would suggest that we put OneCall3 support into PyOWM 4.0 and encourage users to upgrade their PyOWM versions and adapt their clients.
Unfortunately I really don't have spare time to dedicate to this - I'm in a crazily busy moment of my life. But I will eventually put effort into this.

For the moment, if you and @jallen92 have capacity, let me just set up 4.0 up as a new milestone for PyOWM

What do you think guys? And of course, anyone else interested

from pyowm.

KeithB avatar KeithB commented on June 9, 2024

@csparpa I'm happy to get the changes set and deal with wiki updates for sample usage etc. If there are any other process parts I might have missed (preferred coding styles, code review approaches etc) let me know.

Assuming a minor version change I was going to add support for OneCall3 but default to the original OneCall1 endpoint unless specifically configured. On the basis a major version change could be breaking I'll go the other way - default behaviour is OneCall3 but option for clients to configure for legacy endpoints.

Only other bit to call out is any regression testing I can do will be limited as I don't have access to any OneCall1 endpoints.

from pyowm.

m0n1ker avatar m0n1ker commented on June 9, 2024

Hi everyone. I found this issue from the HA issue and forum post and would be happy to help out. Aside from an unrelated bug on Windows I was able to get this project cloned and tox running.

If the plan is to do a major version bump that could introduce breaking changes, would it make sense to pull OneCall out into its own namespace and create a separate manager for it? For example instead of putting OneCall functions inside of owm.weather_manager(), should we create own.onecall_manager()?

I know the majority of data in the OneCall API fits in the existing weather classes, but if we're saying OneCall 3.0 is supported I think we should also have support for the National weather alerts it provides. It also feels like OneCall is slowly diverging from the Weather 2.5 endpoints and this would allow us to more drastically alter the parsers without impacting other APIs as they add features or modify the data structure.

from pyowm.

ncd7 avatar ncd7 commented on June 9, 2024

Any news on a potential fix? Thanks.

from pyowm.

hairlesshobo avatar hairlesshobo commented on June 9, 2024

I hate to be a "me too" kinda person, but I found my way here because I am unable to use the OneCall 3.0 subscription in Home Assistant. I was wondering if there was any update here or perhaps a roadmap regarding implementing support for the v3.0 api?

from pyowm.

csparpa avatar csparpa commented on June 9, 2024

@hairlesshobo @ncd7 @KeithB unfortunately I'm unable to keep up with Pyowm 😣

Can any of you folks can take the lead on this ?

from pyowm.

hairlesshobo avatar hairlesshobo commented on June 9, 2024

At the moment, I unfortunately do not have the spare time because I am still in the middle of a major family move, but if no one else picks this up in the next month or two, I will see if I can submit a PR to add support for the new 3.0 API.

from pyowm.

KeithB avatar KeithB commented on June 9, 2024

I should be able to find the time to prove/update the original fix I had against current OWM functionality over the next 2 to 3 weeks.

I'm happy with that and to look at other tweaks/fixes but don't expect to have the time to do major overhauls. Much as I like the idea of the bigger re-write and starting the 4.x.x branch @m0n1ker mentioned, I need to be realistic about the combination of time and my (minimal) Python abilities.

from pyowm.

tarrant33 avatar tarrant33 commented on June 9, 2024

I should be able to find the time to prove/update the original fix I had against current OWM functionality over the next 2 to 3 weeks.

I'm happy with that and to look at other tweaks/fixes but don't expect to have the time to do major overhauls. Much as I like the idea of the bigger re-write and starting the 4.x.x branch @m0n1ker mentioned, I need to be realistic about the combination of time and my (minimal) Python abilities.

Were you ever able to get this to work with the one call 3.0? Or find a work around?

from pyowm.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.