Update fix/clean up #4179
Update fix/clean up #4179
Conversation
Updates from airqo staging
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive update to the AirQo ETL utilities, focusing on device network categorization and data processing. The changes primarily revolve around introducing a new Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/workflows/airqo_etl_utils/constants.py (1)
50-74: Well-structured DeviceNetwork enum implementation.The implementation is clean, well-documented, and follows Python enum best practices. The error handling for invalid networks adds robustness to the code.
However, consider adding a class-level docstring that includes examples of usage to make it even more developer-friendly.
class DeviceNetwork(Enum): """ METONE -> Us embassy AIRQO -> Airqo URBANBETTER -> Urban Better IQAIR -> Iqair + + Example usage: + network = DeviceNetwork.AIRQO + network_str = str(network) # Returns "airqo" """src/workflows/airqo_etl_utils/airqo_utils.py (2)
429-431: Consider adding error handling for invalid device networks.The
get_devices_by_networkcall should handle cases where an invaliddevice_networkis provided.Add validation before the API call:
+ if device_network and not isinstance(device_network, DeviceNetwork): + logger.warning(f"Invalid device_network provided: {device_network}") + return devices_data devices = airqo_api.get_devices_by_network( device_network=device_network, device_category=device_category )
905-905: Consistent use of DeviceNetwork enum.Good use of the DeviceNetwork enum for network specification, but the string literal "airqo" is still used in the
get_maintenance_logscall.Apply this diff to maintain consistency:
devices = airqo_api.get_devices(network=DeviceNetwork.AIRQO) devices_history = pd.DataFrame() for device in devices: try: maintenance_logs = airqo_api.get_maintenance_logs( - network="airqo", + network=DeviceNetwork.AIRQO, device=device.get("name", None), activity_type="deployment", )Also applies to: 910-910
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/workflows/airqo_etl_utils/airnow_utils.py(5 hunks)src/workflows/airqo_etl_utils/airqo_api.py(5 hunks)src/workflows/airqo_etl_utils/airqo_utils.py(8 hunks)src/workflows/airqo_etl_utils/constants.py(1 hunks)src/workflows/airqo_etl_utils/data_validator.py(1 hunks)src/workflows/dags/airqo_bam_measurements.py(3 hunks)src/workflows/dags/airqo_measurements.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/workflows/airqo_etl_utils/data_validator.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (10)
src/workflows/dags/airqo_bam_measurements.py (2)
13-13: LGTM! Enhanced device filtering with network specification.The addition of
DeviceNetworkimport and the new parameters inextract_bam_dataimprove the precision of device selection, aligning well with the PR objectives.Also applies to: 34-35
94-95: LGTM! Consistent implementation in realtime DAG.The realtime measurements DAG follows the same pattern of network specification, maintaining consistency across the codebase.
src/workflows/airqo_etl_utils/airnow_utils.py (3)
7-7: LGTM! Improved device filtering with network specification.The implementation now correctly filters METONE devices using the new
DeviceNetworkenum, enhancing the precision of device selection.Also applies to: 71-73
128-129: Good code organization improvements.The variable renaming from
device_idtodevice_id_and the relocation ofpollutant_valueinitialization improve code readability.Also applies to: 139-140
145-145: LGTM! Clear error handling and logging.The error messages are descriptive and the logging implementation follows best practices, making debugging easier.
Also applies to: 156-156
src/workflows/dags/airqo_measurements.py (2)
18-18: LGTM! Consistent import structure.The addition of
DeviceNetworkimport aligns with the codebase-wide refactoring approach.
572-572: LGTM! Enhanced device filtering in gaseous measurements DAG.The addition of
device_networkparameter withDeviceNetwork.AIRQOmaintains consistency with other DAGs while improving device selection precision.src/workflows/airqo_etl_utils/airqo_api.py (2)
289-297: Good error handling for network fetching.The code properly handles the case where network fetching fails, logging the error and returning an empty devices list.
29-30: Verify the impact of enabling staging environment storage.The commented code previously prevented storing measurements in the staging environment. This change enables measurement storage in staging, which could affect system resources and data integrity.
Run this script to assess the potential impact:
✅ Verification successful
Enabling staging measurement storage is safe and beneficial
The change allows proper testing of measurement storage functionality in the staging environment while maintaining environment isolation through separate infrastructure configurations. Other staging environment protections, like preventing notifications, remain in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any existing measurements in staging rg -A 3 "staging.*measurements"Length of output: 33
Script:
#!/bin/bash # Search for staging environment configuration and base URL rg "AIRQO_BASE_URL" -A 3 # Search for measurement-related code rg "def.*measurement" -A 5 # Look for staging environment checks rg "if.*staging" -A 3 # Find configuration files fd -e yaml -e yml -e conf -e config -e envLength of output: 37596
src/workflows/airqo_etl_utils/airqo_utils.py (1)
407-407: Good addition ofdevice_networkparameter.The addition of the
device_networkparameter and the updated docstring improves the clarity of the function's purpose and aligns with the network-based filtering functionality.Also applies to: 413-413
| @@ -41,7 +41,7 @@ def save_events(self, measurements: List) -> None: | |||
| ) | |||
|
|
|||
| def get_maintenance_logs( | |||
| self, tenant: str, device: str, activity_type: str = None | |||
| self, network: str, device: str, activity_type: str = None | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parameter rename from tenant to network needs documentation update.
The parameter has been renamed from tenant to network, but the docstring still references tenant. This inconsistency could confuse future developers.
Apply this diff to update the documentation:
"""
Retrieve devices given a tenant and device category.
Args:
- - tenant: An Enum that represents site ownership.
+ - network: A string that represents the device network/manufacturer.
- device: The name of the device.
- activity_type: Defines if the activity logged is a maintenance or deployment activity. If not supplied returns all activities for the given device.Also applies to: 73-73
| self, device_network: str = None, device_category: DeviceCategory = None | ||
| ) -> List[Dict[str, Any]]: | ||
| """ | ||
| Retrieve devices by network based on the specified device category. | ||
|
|
||
| Args: device_category (DeviceCategory, optional): The category of devices to retrieve. Defaults to `DeviceCategory.LOW_COST`. | ||
| Args: | ||
| network (str): This defines the network or manufacture of the device(s) to retrieve. Defaults to `None`. If not passed, devices from all networks are returned. | ||
| device_category (DeviceCategory, optional): The category of devices to retrieve. Defaults to `None`. If not passed, devices from all categories are returned. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Documentation improvement needed for device_network parameter.
The docstring has a mismatch between the parameter name in the signature (device_network) and the documentation (network).
Apply this diff to fix the documentation:
"""
Retrieve devices by network based on the specified device category.
Args:
- network (str): This defines the network or manufacture of the device(s) to retrieve. Defaults to `None`. If not passed, devices from all networks are returned.
+ device_network (str): This defines the network or manufacturer of the device(s) to retrieve. Defaults to `None`. If not passed, devices from all networks are returned.
device_category (DeviceCategory, optional): The category of devices to retrieve. Defaults to `None`. If not passed, devices from all categories are returned.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self, device_network: str = None, device_category: DeviceCategory = None | |
| ) -> List[Dict[str, Any]]: | |
| """ | |
| Retrieve devices by network based on the specified device category. | |
| Args: device_category (DeviceCategory, optional): The category of devices to retrieve. Defaults to `DeviceCategory.LOW_COST`. | |
| Args: | |
| network (str): This defines the network or manufacture of the device(s) to retrieve. Defaults to `None`. If not passed, devices from all networks are returned. | |
| device_category (DeviceCategory, optional): The category of devices to retrieve. Defaults to `None`. If not passed, devices from all categories are returned. | |
| self, device_network: str = None, device_category: DeviceCategory = None | |
| ) -> List[Dict[str, Any]]: | |
| """ | |
| Retrieve devices by network based on the specified device category. | |
| Args: | |
| device_network (str): This defines the network or manufacturer of the device(s) to retrieve. Defaults to `None`. If not passed, devices from all networks are returned. | |
| device_category (DeviceCategory, optional): The category of devices to retrieve. Defaults to `None`. If not passed, devices from all categories are returned. |
Description
This PR allows to pick specific network devices and cleans up the field naming to match the fields returned by get_devices_by_network.
Related Issues
Summary by CodeRabbit
New Features
DeviceNetworkconstant to categorize devices by network typeRefactor
TenanttoDeviceNetworkacross multiple utility filesChores