Implement WeatherForecast telemetry #2

couger01 · 2022-11-03T22:37:33Z

Add eups and pyproject.toml support
Add pre-commit support
Add doc template
Add Jenkinsfile
Add scons boilerplate
Move plantuml files to doc folder
Remove development python files
Add CSC unit test
Add LICENSE

* Add eups and pyproject.toml support * Add pre-commit support * Add doc template * Add Jenkinsfile * Add scons boilerplate * Move plantuml files to doc folder * Remove development python files * Add CSC unit test * Add LICENSE

wvreeven

This is an improvement over the previous code base, but it can be improved much more. I will probably have more comments once you have processed the current ones.

wvreeven · 2022-11-04T12:36:57Z

.pre-commit-config.yaml

@@ -0,0 +1,9 @@
+repos:


Please follow the instructions on https://confluence.lsstcorp.org/display/LTS/TSSW+pre-commit+hooks and add check-yaml and isort.

wvreeven · 2022-11-04T12:39:35Z

doc/conf.py

@@ -0,0 +1,11 @@
+"""Sphinx configuration file for TSSW package"""
+
+from documenteer.conf.pipelinespkg import *


Please include an import for the python package of this project.

I see you didn't add the import for the python package of this project. If you don't want to include it or see no need for it, please leave a comment explaining why.

wvreeven · 2022-11-04T12:41:36Z

doc/developer-guide/developer-guide.rst

+
+[This section should list dependencies in bullet form]
+
+* `SAL <https://ts-sal.lsst.io>`_ - v4.0.0


We currently are on sal v7.0.0

wvreeven · 2022-11-04T12:41:51Z

doc/developer-guide/developer-guide.rst

+[This section should list dependencies in bullet form]
+
+* `SAL <https://ts-sal.lsst.io>`_ - v4.0.0
+* ts_salobj - v5.2.0


We currently are on salobj v7.2.0

wvreeven · 2022-11-04T12:43:20Z

doc/requirements.txt

@@ -0,0 +1 @@
+documenteer[pipelines]


This file may be removed.

wvreeven · 2022-11-04T13:09:31Z

python/lsst/ts/weatherforecast/csc.py

+                async with session.get(
+                    "/packages/trendpro-1h_trendpro-day", params=params
+                ) as resp:
+                    with open("forecast.json", "wb") as fd:


Please rewrite this and the next few lines to store the contents of the file in memory instead of relying on writing to disk.

wvreeven · 2022-11-04T13:22:52Z

python/lsst/ts/weatherforecast/csc.py

+                "referenceevapotranspiration_fao"
+            ],
+        )
+        await asyncio.sleep(12 * 3600)


Please extract to a config option, or at least a constant, and use that here.

wvreeven · 2022-11-04T13:23:42Z

python/lsst/ts/weatherforecast/csc.py

+            with open("forecast.json") as f:
+                df = json.load(f)
+        metadata_fld = df["metadata"]
+        modelrun_utc = datetime.datetime.strptime(


I think you can simply use the datetime.datetime.timestamp() function instead and ditch the calendar import. You appear to do this conversion several times below so extract to a function and call that here and everywhere else.

Can you please explain why you didn't decide to do this in a comment to this conversation?

I missed it.

wvreeven · 2022-11-04T13:24:48Z

tests/test_csc.py

+            assert approx(-70.34) == metadata.longitude
+            assert 2298 == metadata.height
+            assert "GMT-03" == metadata.timezoneAbbrevation
+            assert -3 == metadata.timeOffset


Please add functions that test the other telemetry as well.

wvreeven · 2022-11-04T13:25:55Z

ups/ts_weatherforecast.table

@@ -0,0 +1,2 @@
+envPrepend(PYTHONPATH, ${PRODUCT_DIR}/python)


Please add dependencies for salobj and utils, otherwise EUPS cannot work properly.

wvreeven

I had another look and found that you didn't address all my previous comments. I added a few new ones too.

wvreeven · 2022-11-07T11:50:06Z

conda/meta.yaml

+    path: ../
+
+build:
+    script: python -m pip install --no-deps --ignore-installed .


Please take a look at this confluence page. It contains instructions for how to modernize the conda recipe.

wvreeven · 2022-11-07T11:51:04Z

doc/conf.py

@@ -0,0 +1,11 @@
+"""Sphinx configuration file for TSSW package"""
+
+from documenteer.conf.pipelinespkg import *


I see you didn't add the import for the python package of this project. If you don't want to include it or see no need for it, please leave a comment explaining why.

wvreeven · 2022-11-07T11:52:11Z

doc/developer-guide/developer-guide.rst

+Usage
+=====
+
+In order to run the CSC, the METEOBLUE_API_KEY ev needs to be set.


ev -> environment variable.

wvreeven · 2022-11-07T11:56:00Z

python/lsst/ts/weatherforecast/__init__.py

@@ -0,0 +1,27 @@
+# This file is part of ts_weatherforecast.
+#
+# Developed for the LSST Telescope and Site Systems.


Please modernize this line. We have been referring to Rubin Observatory for well over a year now. A suggestion for this line would be

# Developed for the Vera C. Rubin Observatory Telescope and Site Systems.

And review all other Python files as well and add a header there or correct the one you added.

Grabbed an old header by accident.

wvreeven · 2022-11-07T12:01:07Z

python/lsst/ts/weatherforecast/csc.py

+FORMAT = "json"
+
+
+class WeatherForecastCSC(salobj.BaseCsc):


Dude, adding a doc string means adding a complete doc string. You didn't describe the parameters, nor the attributes. Also please add a short description of what the CSC is supposed to do and expand CSC to Commandable SAL Component.

wvreeven · 2022-11-07T12:01:46Z

python/lsst/ts/weatherforecast/csc.py

+        self.interval = 12 * 3600
+
+    def convert_time(self, timestamp):
+        """Convert datetime string to unix timestamp


Please end the sentence in a ..

wvreeven · 2022-11-07T12:02:22Z

python/lsst/ts/weatherforecast/csc.py

+        self.last_time = None
+        self.interval = 0
+
+    async def telemetry(self):


Again, please add a short description of what the telemetry loop is supposed to do.

wvreeven · 2022-11-07T12:03:00Z

python/lsst/ts/weatherforecast/csc.py

+            with open("forecast.json") as f:
+                df = json.load(f)
+        metadata_fld = df["metadata"]
+        modelrun_utc = datetime.datetime.strptime(


Can you please explain why you didn't decide to do this in a comment to this conversation?

wvreeven

Looks good to me. Thanks for all the changes. I left a few more comments for you too.

wvreeven · 2022-11-07T14:29:00Z

conda/meta.yaml

+        - schema
+        - setup.cfg
+    commands:
+        - py.test


Oops I missed this one. Please change to pytest.

wvreeven · 2022-11-07T14:52:39Z

python/lsst/ts/weatherforecast/csc.py

+from lsst.ts import salobj, utils
+
+API_KEY = os.getenv("METEOBLUE_API_KEY")
+LATITUDE = -30.24


It turns out that the CSC is set to be non-configurable so let's leave it as is.

wvreeven · 2022-11-07T14:54:37Z

python/lsst/ts/weatherforecast/csc.py

+    last_time : `None`
+        The last time that the forecast was updated.
+    interval : `int`
+        The interval at which the telemetry loop is run.


Please add the unit here too.

wvreeven · 2022-11-07T14:54:59Z

python/lsst/ts/weatherforecast/csc.py

+    telemetry_task : `asyncio.Future`
+        A task that handles the telemetry loop.
+    last_time : `None`
+        The last time that the forecast was updated.


Please indicate that the time is in UTC.

* Add COPYRIGHT and README * Add License header * Fill out basic documentation * Move json to data folder under python package * Add dependencies to Eups * Add conda recipe * Update conda recipe * Fix header * Complete doc string * Remove calendar import

Implement WeatherForecast telemetry

e29636e

* Add eups and pyproject.toml support * Add pre-commit support * Add doc template * Add Jenkinsfile * Add scons boilerplate * Move plantuml files to doc folder * Remove development python files * Add CSC unit test * Add LICENSE

couger01 requested a review from wvreeven November 3, 2022 22:37

wvreeven requested changes Nov 4, 2022

View reviewed changes

couger01 force-pushed the tickets/DM-36703 branch 2 times, most recently from 7ba5e83 to 01ad6e2 Compare November 6, 2022 19:54

wvreeven requested changes Nov 7, 2022

View reviewed changes

couger01 requested a review from wvreeven November 7, 2022 13:23

wvreeven approved these changes Nov 7, 2022

View reviewed changes

Complete telemetry unit tests

efbc573

* Add COPYRIGHT and README * Add License header * Fill out basic documentation * Move json to data folder under python package * Add dependencies to Eups * Add conda recipe * Update conda recipe * Fix header * Complete doc string * Remove calendar import

couger01 force-pushed the tickets/DM-36703 branch from 39baa09 to efbc573 Compare November 7, 2022 15:48

couger01 merged commit 2b63f28 into develop Nov 7, 2022

couger01 deleted the tickets/DM-36703 branch November 7, 2022 15:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement WeatherForecast telemetry #2

Implement WeatherForecast telemetry #2

couger01 commented Nov 3, 2022

wvreeven left a comment

wvreeven Nov 4, 2022

wvreeven Nov 4, 2022

wvreeven Nov 7, 2022

wvreeven Nov 4, 2022

wvreeven Nov 4, 2022

wvreeven Nov 4, 2022

wvreeven Nov 4, 2022

wvreeven Nov 4, 2022

wvreeven Nov 4, 2022

wvreeven Nov 7, 2022

couger01 Nov 7, 2022

wvreeven Nov 4, 2022

wvreeven Nov 4, 2022

wvreeven left a comment

wvreeven Nov 7, 2022

wvreeven Nov 7, 2022

wvreeven Nov 7, 2022

wvreeven Nov 7, 2022

couger01 Nov 7, 2022

wvreeven Nov 7, 2022

wvreeven Nov 7, 2022

wvreeven Nov 7, 2022

wvreeven Nov 7, 2022

wvreeven left a comment

wvreeven Nov 7, 2022

wvreeven Nov 7, 2022

wvreeven Nov 7, 2022

wvreeven Nov 7, 2022

		@@ -0,0 +1,11 @@
		"""Sphinx configuration file for TSSW package"""

		from documenteer.conf.pipelinespkg import *


		[This section should list dependencies in bullet form]

		* `SAL <https://ts-sal.lsst.io>`_ - v4.0.0

		@@ -0,0 +1,2 @@
		envPrepend(PYTHONPATH, ${PRODUCT_DIR}/python)

		@@ -0,0 +1 @@
		documenteer[pipelines]

		FORMAT = "json"


		class WeatherForecastCSC(salobj.BaseCsc):

Implement WeatherForecast telemetry #2

Implement WeatherForecast telemetry #2

Conversation

couger01 commented Nov 3, 2022

wvreeven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wvreeven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wvreeven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment