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

Station ID is None in error-log & log-level to high #36

Closed
Weidav opened this issue Jul 25, 2024 · 6 comments
Closed

Station ID is None in error-log & log-level to high #36

Weidav opened this issue Jul 25, 2024 · 6 comments
Assignees
Labels
bug Something isn't working

Comments

@Weidav
Copy link
Contributor

Weidav commented Jul 25, 2024

When the "get_data" method is used with the new "station_ids" argument, then the variable "station_id" is None when logged in this line: ndbc_api.py:471
If that's an easy fix for you, I'd be happy to see that fixed in the next release, otherwise I can have a look at it in the near future and fix it myself.

I also think that logging this message at error level is not necessary when downloading data from multiple stations, as it is expected that some requested stations are offline and will fail. At least in my case I always get some error logs because of this. Let me know what you think.

And thanks again for the possibility do download data from multiple stations at once, that drastically speeds up my download!

@CDJellen CDJellen self-assigned this Jul 25, 2024
@CDJellen CDJellen added the bug Something isn't working label Jul 25, 2024
@CDJellen
Copy link
Owner

Hello @Weidav ; thank you for opening this issue. It should be relatively straightforward to improve the logging to account for multi-station download support. @abdu558 suggested and assisted in developing download parallelism; let me know if you have any other suggestions that could improve the API's fuctionality!

Have an excellent rest of your day, I'll open a PR with a fix and push a new version to PyPi and Conda over the next few days.

@Weidav
Copy link
Contributor Author

Weidav commented Jul 25, 2024

Thanks for the quick response! The rest of the library looks great to me.

@CDJellen
Copy link
Owner

Hello @Weidav ; this bug has been addressed in the latest release v2024.07.28.1 which should be available through PyPi and Conda forge shortly. Thanks again and have a great rest of your day!

@Weidav
Copy link
Contributor Author

Weidav commented Jul 31, 2024

Thanks for your quick work!

What do you think about my suggestion of decreasing the log-level of this message to "warning"?
As mentioned, I think that it is expected that some stations are offline and fail. But I could also be wrong and misinterpret this message. Are the buoys mentioned in this message removed and never back online?

There are many buoys that are in and out of the water depending on the season. If they are included in this message, then I think I have a strong argument to decrease the log-level. If it only includes buoys that are permanently removed, then error is appropriate.

@CDJellen
Copy link
Owner

CDJellen commented Aug 2, 2024

Thanks for your quick work!

What do you think about my suggestion of decreasing the log-level of this message to "warning"? As mentioned, I think that it is expected that some stations are offline and fail. But I could also be wrong and misinterpret this message. Are the buoys mentioned in this message removed and never back online?

There are many buoys that are in and out of the water depending on the season. If they are included in this message, then I think I have a strong argument to decrease the log-level. If it only includes buoys that are permanently removed, then error is appropriate.

At minimum I'd like to ensure this is easier to configure.

The logging for the package overall likely could use a refactor in light of the change to get_data.

Depending on the scope of the changes I will either push a release which reduces the log verbosity, or a more significant release which makes log level configuration easier and more malleable. In either case, I'll try to have the improvement released over the next three days.

Thanks again and have an excellent rest of your day!

@CDJellen CDJellen reopened this Aug 2, 2024
@CDJellen
Copy link
Owner

CDJellen commented Aug 4, 2024

Hello @Weidav ; I've made the updates in the latest PR #39

The default log level is now logging.ERROR but this can be configured at initialization or by calling api.configure_logging(). Additionally, logging is now structured and supports logging to a file.

Thanks and have an excellent rest of your day, the PyPi release should be available shortly at version v2024.08.04.1.

@CDJellen CDJellen closed this as completed Aug 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants