HTTP Utils and PathBuilder mods #85
Open
+1,433
−664
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Create a generic HTTP class based on the aws_utils with similar issue/valid time handling to deal with possible observation datasets.
mackenzie-grimes-noaa
previously approved these changes
Nov 4, 2024
Geary-Layne
requested changes
Nov 13, 2024
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| # pylint: disable=duplicate-code, broad-exception-caught |
There was a problem hiding this comment.
Suggested change
| # pylint: disable=duplicate-code, broad-exception-caught | |
| # pylint: disable=duplicate-code, broad-exception-caught |
If we have to disable linter we want to have as small of a scope as possible. So 'broad-exception-caught' should be for the specific line of code. I'm guessing that the 'duplicate-code' is because this module is very similar to the aws_utils.py, if that is the case maybe we should make a recourse_utils.py for the common and both aws_utils and http_utils extend that.
There was a problem hiding this comment.
I raised the possibility of an interface initially, many of the calls made by DAS would be the same for http and aws, at this point it can probably wait for a while.
There was a problem hiding this comment.
With the refactor we don't need to disable duplicate code, and broad exception should be applied directly to the except and not globally, and ideally we want to specify the specific exception(s) that might happen
Co-authored-by: Geary Layne <[email protected]>
Co-authored-by: Geary Layne <[email protected]>
Co-authored-by: Geary Layne <[email protected]>
I've added a protocol_utils.py as the base class and updated the aws_utils, http_utils code accordingly. |
Geary-Layne
previously approved these changes
Dec 3, 2024
| @@ -361,6 +361,9 @@ def parse_args(key: str, value: str, result: dict): | |||
| for i, _dir in enumerate(dirs): | |||
| res = re.search(r'{.*}', _dir) | |||
| if res: | |||
| parse_args(res.group(), vals[i][res.span()[0]:], time_args) | |||
| try : | |||
There was a problem hiding this comment.
I'll need to incorporate this exception handling in the new implementation of path builder, thanks for finding/fixing. Is there a test that exercises this code?
There was a problem hiding this comment.
My test was running it against the MRMS http feed so no.
| assert len(result) == 1 | ||
| assert result[0] == EXAMPLE_ISSUE | ||
|
|
||
| def test_get_valids_all(http_utils: HttpUtils, httpserver: HTTPServer): |
There was a problem hiding this comment.
Given that this http_utils.get_valids doesn't find anything, is this a meaningful test?
There was a problem hiding this comment.
This does return a list of files based on the pattern provided so I feel that this is handled.
There was a problem hiding this comment.
This return an empty list because the sample filename you used don't have and valid info. If the files were structured list this:
'MRMS_MergedReflectivityQC_00.50_{issue.year:04d}{issue.month:02d}{issue.day:02d}'
'-{issue.hour:02d}{issue.minute:02d}{issue.second:02d}'_{valid.year:04d}{valid.month:02d}'
'{valid.day:02d}-{valid.hour:02d}{valid.minute:02d}{valid.second:02d}.grib2.gz'
It should return a valid datetime that matches the issue. We want http_utils to be generic enough to handle more than MRMS so we need to test that it can get valids.
| #assert result[0] == (EXAMPLE_VALID, f'{EXAMPLE_URL}{EXAMPLE_PROD_DIR}{EXAMPLE_FILES[1]}') | ||
|
|
||
|
|
||
| def test_get_valids_with_wildcards(http_utils_with_wild: HttpUtils, httpserver: HTTPServer): |
There was a problem hiding this comment.
Given that this http_utils_with_wild.get_valids doesn't find anything, is this a meaningful test?
There was a problem hiding this comment.
True, I'll add a meaningful test...
There was a problem hiding this comment.
Again if the filepath format contained valid this should return something. If you want, I'd be okay if we dropped this test unless it is need for code coverage. If you do drop it make sure to remove http_utils_with_wild feature if it isn't used anywhere else.
Co-authored-by: Geary Layne <[email protected]>
Co-authored-by: Geary Layne <[email protected]>
Co-authored-by: Geary Layne <[email protected]>
Co-authored-by: Geary Layne <[email protected]>
Co-authored-by: Geary Layne <[email protected]>
Co-authored-by: Geary Layne <[email protected]>
|
If I merge these changes into main they will break DAS. I can make those
DAS changes when I put in the new MRMS code.
…On Wed, Dec 4, 2024 at 2:31 PM Geary Layne ***@***.***> wrote:
***@***.**** approved this pull request.
Please make changes. If everything gets cleared up feel free to merge, or
if you would like I can review it again.
------------------------------
In python/idsse_common/test/test_http_utils.py
<#85 (comment)>
:
> @@ -0,0 +1,157 @@
+"""Test suite for http_utils.py"""
+# ----------------------------------------------------------------------------------
+# Created on Tue Dec 3
+#
+# Copyright (c) 2023 Colorado State University. All rights reserved. (1)
+# Copyright (c) 2023 Regents of the University of Colorado. All rights reserved. (2)
+#
+# Contributors:
+# Paul Hamer (1)
+#
+# ----------------------------------------------------------------------------------
+# pylint: disable=missing-function-docstring,redefined-outer-name,pointless-statement
+# pylint: disable=invalid-name,unused-argument
⬇️ Suggested change
-# pylint: disable=invalid-name,unused-argument
+# pylint: disable=invalid-name,unused-argument,duplicate-code,line-too-long
------------------------------
In python/idsse_common/test/test_http_utils.py
<#85 (comment)>
:
> +from idsse.testing.utils.resources import get_resource_from_file
+
+
+EXAMPLE_ISSUE = datetime(2024, 10, 30, 20, 54, 38, tzinfo=UTC)
+EXAMPLE_VALID = datetime(2024, 10, 30, 20, 54, 38, tzinfo=UTC)
+
+EXAMPLE_URL = 'http://127.0.0.1:5000/data/'
+EXAMPLE_ENDPOINT = '3DRefl/MergedReflectivityQC_00.50'
+EXAMPLE_PROD_DIR = '3DRefl/MergedReflectivityQC_00.50/'
+EXAMPLE_FILES = ['MRMS_MergedReflectivityQC_00.50.latest.grib2.gz',
+ 'MRMS_MergedReflectivityQC_00.50_20241030-205438.grib2.gz',
+ 'MRMS_MergedReflectivityQC_00.50_20241030-205640.grib2.gz']
+EXAMPLE_RETURN = get_resource_from_file('idsse.testing.idsse_common',
+ 'mrms_response.html')
+
+# pylint: disable=duplicate-code, line-too-long
⬇️ Suggested change
-# pylint: disable=duplicate-code, line-too-long
------------------------------
In python/idsse_common/idsse/common/protocol_utils.py
<#85 (comment)>
:
> + file_ext: str) -> None:
+ self.path_builder = PathBuilder(basedir, subdir, file_base, file_ext)
+
+ # pylint: disable=invalid-name
+ @AbstractMethod
+ def ls(self, path: str, prepend_path: bool = True) -> Sequence[str]:
+ """Execute a 'ls' on the specified path
+
+ Args:
+ path (str): path
+ prepend_path (bool): Add to the filename
+
+ Returns:
+ Sequence[str]: The results sent to stdout from executing a 'ls' on passed path
+ """
+
I think it makes sense to add a 'cd' abstract method, since both aws and
http implement them.
------------------------------
In python/idsse_common/idsse/common/protocol_utils.py
<#85 (comment)>
:
> + valid_end (datetime | None, optional): All returned objects will be for
+ valids <= valid_end. Defaults to None.
+
+ Returns:
+ Sequence[tuple[datetime, str]]: A sequence of tuples with valid date/time (indicated by
+ object's location) and the object's location (path).
+ Empty Sequence if no valids found for given time range.
+ """
+ if valid_start and valid_start == valid_end:
+ valids_and_filenames = self.check_for(issue, valid_start)
+ return [valids_and_filenames] if valids_and_filenames is not None else []
+
+ dir_path = self.path_builder.build_dir(issue=issue)
+ valid_and_file = [(self.path_builder.get_valid(file_path), file_path)
+ for file_path in self.ls(dir_path)
+ if file_path.endswith(self.path_builder.file_ext)]
I think there is a bug here, that http_utils exposes. It's might be
possible to get file_paths that don't match with the specified issueDt.
When I tried adding the following suggestion it cause problems with
wildcard, so maybe something different and/or changes elsewhere.
⬇️ Suggested change
- if file_path.endswith(self.path_builder.file_ext)]
+ if file_path.endswith(self.path_builder.file_ext) and
+ self.path_builder.get_issue(file_path] == issue]
------------------------------
In python/idsse_common/idsse/common/aws_utils.py
<#85 (comment)>
:
>
- def aws_ls(self, path: str, prepend_path: bool = True) -> Sequence[str]:
- """Execute an 'ls' on the AWS s3 bucket specified by path
+ @staticmethod
I think it makes more sense to rename 'aws_ls' to just 'ls' vs having 'ls'
call 'aws_ls'. Of course it will no longer be a static method
------------------------------
In python/idsse_common/idsse/common/aws_utils.py
<#85 (comment)>
:
> @@ -65,20 +59,20 @@ def aws_ls(self, path: str, prepend_path: bool = True) -> Sequence[str]:
return [os.path.join(path, filename.split(' ')[-1]) for filename in commands_result]
return [filename.split(' ')[-1] for filename in commands_result]
- def aws_cp(self,
- path: str,
+ @staticmethod
+ def aws_cp(path: str,
See my suggestion in adding abstract 'cp' to protocal_utils.py, and rename
this to just 'cp' like 'ls' above.
------------------------------
In python/idsse_common/idsse/common/http_utils.py
<#85 (comment)>
:
> +import shutil
+from collections.abc import Sequence
+
+import requests
+
+from .protocol_utils import ProtocolUtils
+
+logger = logging.getLogger(__name__)
+
+# pylint: disable=duplicate-code, broad-exception-caught
+
+class HttpUtils(ProtocolUtils):
+ """http Utility Class - Used by DAS for file downloads"""
+
+ def ls(self, path: str, prepend_path: bool = True) -> Sequence[str]:
+ """Execute a 'ls' on the AWS s3 bucket specified by path
Please update the doc string here and anywhere else in the file to be
specific to http.
------------------------------
In python/idsse_common/idsse/common/http_utils.py
<#85 (comment)>
:
> +
+ def ls(self, path: str, prepend_path: bool = True) -> Sequence[str]:
+ """Execute a 'ls' on the AWS s3 bucket specified by path
+
+ Args:
+ path (str): s3 bucket
+ prepend_path (bool): Add to the filename
+
+ Returns:
+ Sequence[str]: The results sent to stdout from executing a 'ls' on passed path
+ """
+ return self.http_ls(path, prepend_path)
+
+
+ @staticmethod
+ def http_ls(url: str, prepend_path: bool = True) -> Sequence[str]:
Similar to aws_utils, just have 'ls' and 'cp' implement abstract methods
directly.
------------------------------
In python/idsse_common/idsse/common/http_utils.py
<#85 (comment)>
:
> +
+import logging
+import fnmatch
+import os
+import shutil
+from collections.abc import Sequence
+from datetime import datetime, timedelta, UTC
+
+import requests
+
+from .path_builder import PathBuilder
+from .utils import TimeDelta, datetime_gen
+
+logger = logging.getLogger(__name__)
+
+# pylint: disable=duplicate-code, broad-exception-caught
With the refactor we don't need to disable duplicate code, and broad
exception should be applied directly to the except and not globally, and
ideally we want to specify the specific exception(s) that might happen
------------------------------
In python/idsse_common/test/test_http_utils.py
<#85 (comment)>
:
> + url = '/data/' + EXAMPLE_ENDPOINT +'/'
+ httpserver.expect_request(url).respond_with_data(EXAMPLE_RETURN, content_type="text/plain")
+ result = http_utils.get_issues(issue_start=EXAMPLE_ISSUE,
+ time_delta=timedelta(minutes=1))
+ assert len(result) == 1
+ assert result[0] == EXAMPLE_ISSUE
+
+
+def test_get_issues_with_same_start_stop(http_utils: HttpUtils, httpserver: HTTPServer):
+ url = '/data/'+EXAMPLE_ENDPOINT+'/'
+ httpserver.expect_request(url).respond_with_data(EXAMPLE_RETURN, content_type="text/plain")
+ result = http_utils.get_issues(issue_start=EXAMPLE_ISSUE, issue_end=EXAMPLE_ISSUE, time_delta=timedelta(minutes=1))
+ assert len(result) == 1
+ assert result[0] == EXAMPLE_ISSUE
+
+def test_get_valids_all(http_utils: HttpUtils, httpserver: HTTPServer):
This return an empty list because the sample filename you used don't have
and valid info. If the files were structured list this:
'MRMS_MergedReflectivityQC_00.50_{issue.year:04d}{issue.month:02d}{issue.day:02d}'
'-{issue.hour:02d}{issue.minute:02d}{issue.second:02d}'_{valid.year:04d}{valid.month:02d}'
'{valid.day:02d}-{valid.hour:02d}{valid.minute:02d}{valid.second:02d}.grib2.gz'
It should return a valid datetime that matches the issue. We want
http_utils to be generic enough to handle more than MRMS so we need to test
that it can get valids.
------------------------------
In python/idsse_common/test/test_http_utils.py
<#85 (comment)>
:
> +def test_get_issues_with_same_start_stop(http_utils: HttpUtils, httpserver: HTTPServer):
+ url = '/data/'+EXAMPLE_ENDPOINT+'/'
+ httpserver.expect_request(url).respond_with_data(EXAMPLE_RETURN, content_type="text/plain")
+ result = http_utils.get_issues(issue_start=EXAMPLE_ISSUE, issue_end=EXAMPLE_ISSUE, time_delta=timedelta(minutes=1))
+ assert len(result) == 1
+ assert result[0] == EXAMPLE_ISSUE
+
+def test_get_valids_all(http_utils: HttpUtils, httpserver: HTTPServer):
+ url = '/data/'+EXAMPLE_ENDPOINT+'/'
+ httpserver.expect_request(url).respond_with_data(EXAMPLE_RETURN, content_type="text/plain")
+ result = http_utils.get_valids(EXAMPLE_ISSUE)
+ assert len(result) == 0
+ #assert result[0] == (EXAMPLE_VALID, f'{EXAMPLE_URL}{EXAMPLE_PROD_DIR}{EXAMPLE_FILES[1]}')
+
+
+def test_get_valids_with_wildcards(http_utils_with_wild: HttpUtils, httpserver: HTTPServer):
Again if the filepath format contained valid this should return something.
If you want, I'd be okay if we dropped this test unless it is need for code
coverage. If you do drop it make sure to remove http_utils_with_wild
feature if it isn't used anywhere else.
—
Reply to this email directly, view it on GitHub
<#85 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ATO5PJLTF6ROC6NHIOA2JH32D5YDHAVCNFSM6AAAAABRBCNJVOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDINZZHA4TGOJYGU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
--
Paul Hamer
<http://www.esrl.noaa.gov/fiqas/>
Global Systems Laboratory (GSL)
325 Broadway R/GS5
Boulder, CO 80305-3328
|
|
Also the new path_builder changes you implemented broke the http_utils.
I'll look at what I need to fix...
On Thu, Dec 5, 2024 at 3:23 PM Paul Hamer - NOAA Affiliate <
***@***.***> wrote:
… If I merge these changes into main they will break DAS. I can make those
DAS changes when I put in the new MRMS code.
On Wed, Dec 4, 2024 at 2:31 PM Geary Layne ***@***.***>
wrote:
> ***@***.**** approved this pull request.
>
> Please make changes. If everything gets cleared up feel free to merge, or
> if you would like I can review it again.
> ------------------------------
>
> In python/idsse_common/test/test_http_utils.py
> <#85 (comment)>
> :
>
> > @@ -0,0 +1,157 @@
> +"""Test suite for http_utils.py"""
> +# ----------------------------------------------------------------------------------
> +# Created on Tue Dec 3
> +#
> +# Copyright (c) 2023 Colorado State University. All rights reserved. (1)
> +# Copyright (c) 2023 Regents of the University of Colorado. All rights reserved. (2)
> +#
> +# Contributors:
> +# Paul Hamer (1)
> +#
> +# ----------------------------------------------------------------------------------
> +# pylint: disable=missing-function-docstring,redefined-outer-name,pointless-statement
> +# pylint: disable=invalid-name,unused-argument
>
> ⬇️ Suggested change
>
> -# pylint: disable=invalid-name,unused-argument
> +# pylint: disable=invalid-name,unused-argument,duplicate-code,line-too-long
>
> ------------------------------
>
> In python/idsse_common/test/test_http_utils.py
> <#85 (comment)>
> :
>
> > +from idsse.testing.utils.resources import get_resource_from_file
> +
> +
> +EXAMPLE_ISSUE = datetime(2024, 10, 30, 20, 54, 38, tzinfo=UTC)
> +EXAMPLE_VALID = datetime(2024, 10, 30, 20, 54, 38, tzinfo=UTC)
> +
> +EXAMPLE_URL = 'http://127.0.0.1:5000/data/'
> +EXAMPLE_ENDPOINT = '3DRefl/MergedReflectivityQC_00.50'
> +EXAMPLE_PROD_DIR = '3DRefl/MergedReflectivityQC_00.50/'
> +EXAMPLE_FILES = ['MRMS_MergedReflectivityQC_00.50.latest.grib2.gz',
> + 'MRMS_MergedReflectivityQC_00.50_20241030-205438.grib2.gz',
> + 'MRMS_MergedReflectivityQC_00.50_20241030-205640.grib2.gz']
> +EXAMPLE_RETURN = get_resource_from_file('idsse.testing.idsse_common',
> + 'mrms_response.html')
> +
> +# pylint: disable=duplicate-code, line-too-long
>
> ⬇️ Suggested change
>
> -# pylint: disable=duplicate-code, line-too-long
>
> ------------------------------
>
> In python/idsse_common/idsse/common/protocol_utils.py
> <#85 (comment)>
> :
>
> > + file_ext: str) -> None:
> + self.path_builder = PathBuilder(basedir, subdir, file_base, file_ext)
> +
> + # pylint: disable=invalid-name
> + @AbstractMethod
> + def ls(self, path: str, prepend_path: bool = True) -> Sequence[str]:
> + """Execute a 'ls' on the specified path
> +
> + Args:
> + path (str): path
> + prepend_path (bool): Add to the filename
> +
> + Returns:
> + Sequence[str]: The results sent to stdout from executing a 'ls' on passed path
> + """
> +
>
> I think it makes sense to add a 'cd' abstract method, since both aws and
> http implement them.
> ------------------------------
>
> In python/idsse_common/idsse/common/protocol_utils.py
> <#85 (comment)>
> :
>
> > + valid_end (datetime | None, optional): All returned objects will be for
> + valids <= valid_end. Defaults to None.
> +
> + Returns:
> + Sequence[tuple[datetime, str]]: A sequence of tuples with valid date/time (indicated by
> + object's location) and the object's location (path).
> + Empty Sequence if no valids found for given time range.
> + """
> + if valid_start and valid_start == valid_end:
> + valids_and_filenames = self.check_for(issue, valid_start)
> + return [valids_and_filenames] if valids_and_filenames is not None else []
> +
> + dir_path = self.path_builder.build_dir(issue=issue)
> + valid_and_file = [(self.path_builder.get_valid(file_path), file_path)
> + for file_path in self.ls(dir_path)
> + if file_path.endswith(self.path_builder.file_ext)]
>
> I think there is a bug here, that http_utils exposes. It's might be
> possible to get file_paths that don't match with the specified issueDt.
> When I tried adding the following suggestion it cause problems with
> wildcard, so maybe something different and/or changes elsewhere.
> ⬇️ Suggested change
>
> - if file_path.endswith(self.path_builder.file_ext)]
> + if file_path.endswith(self.path_builder.file_ext) and
> + self.path_builder.get_issue(file_path] == issue]
>
> ------------------------------
>
> In python/idsse_common/idsse/common/aws_utils.py
> <#85 (comment)>
> :
>
> >
> - def aws_ls(self, path: str, prepend_path: bool = True) -> Sequence[str]:
> - """Execute an 'ls' on the AWS s3 bucket specified by path
> + @staticmethod
>
> I think it makes more sense to rename 'aws_ls' to just 'ls' vs having
> 'ls' call 'aws_ls'. Of course it will no longer be a static method
> ------------------------------
>
> In python/idsse_common/idsse/common/aws_utils.py
> <#85 (comment)>
> :
>
> > @@ -65,20 +59,20 @@ def aws_ls(self, path: str, prepend_path: bool = True) -> Sequence[str]:
> return [os.path.join(path, filename.split(' ')[-1]) for filename in commands_result]
> return [filename.split(' ')[-1] for filename in commands_result]
>
> - def aws_cp(self,
> - path: str,
> + @staticmethod
> + def aws_cp(path: str,
>
> See my suggestion in adding abstract 'cp' to protocal_utils.py, and
> rename this to just 'cp' like 'ls' above.
> ------------------------------
>
> In python/idsse_common/idsse/common/http_utils.py
> <#85 (comment)>
> :
>
> > +import shutil
> +from collections.abc import Sequence
> +
> +import requests
> +
> +from .protocol_utils import ProtocolUtils
> +
> +logger = logging.getLogger(__name__)
> +
> +# pylint: disable=duplicate-code, broad-exception-caught
> +
> +class HttpUtils(ProtocolUtils):
> + """http Utility Class - Used by DAS for file downloads"""
> +
> + def ls(self, path: str, prepend_path: bool = True) -> Sequence[str]:
> + """Execute a 'ls' on the AWS s3 bucket specified by path
>
> Please update the doc string here and anywhere else in the file to be
> specific to http.
> ------------------------------
>
> In python/idsse_common/idsse/common/http_utils.py
> <#85 (comment)>
> :
>
> > +
> + def ls(self, path: str, prepend_path: bool = True) -> Sequence[str]:
> + """Execute a 'ls' on the AWS s3 bucket specified by path
> +
> + Args:
> + path (str): s3 bucket
> + prepend_path (bool): Add to the filename
> +
> + Returns:
> + Sequence[str]: The results sent to stdout from executing a 'ls' on passed path
> + """
> + return self.http_ls(path, prepend_path)
> +
> +
> + @staticmethod
> + def http_ls(url: str, prepend_path: bool = True) -> Sequence[str]:
>
> Similar to aws_utils, just have 'ls' and 'cp' implement abstract methods
> directly.
> ------------------------------
>
> In python/idsse_common/idsse/common/http_utils.py
> <#85 (comment)>
> :
>
> > +
> +import logging
> +import fnmatch
> +import os
> +import shutil
> +from collections.abc import Sequence
> +from datetime import datetime, timedelta, UTC
> +
> +import requests
> +
> +from .path_builder import PathBuilder
> +from .utils import TimeDelta, datetime_gen
> +
> +logger = logging.getLogger(__name__)
> +
> +# pylint: disable=duplicate-code, broad-exception-caught
>
> With the refactor we don't need to disable duplicate code, and broad
> exception should be applied directly to the except and not globally, and
> ideally we want to specify the specific exception(s) that might happen
> ------------------------------
>
> In python/idsse_common/test/test_http_utils.py
> <#85 (comment)>
> :
>
> > + url = '/data/' + EXAMPLE_ENDPOINT +'/'
> + httpserver.expect_request(url).respond_with_data(EXAMPLE_RETURN, content_type="text/plain")
> + result = http_utils.get_issues(issue_start=EXAMPLE_ISSUE,
> + time_delta=timedelta(minutes=1))
> + assert len(result) == 1
> + assert result[0] == EXAMPLE_ISSUE
> +
> +
> +def test_get_issues_with_same_start_stop(http_utils: HttpUtils, httpserver: HTTPServer):
> + url = '/data/'+EXAMPLE_ENDPOINT+'/'
> + httpserver.expect_request(url).respond_with_data(EXAMPLE_RETURN, content_type="text/plain")
> + result = http_utils.get_issues(issue_start=EXAMPLE_ISSUE, issue_end=EXAMPLE_ISSUE, time_delta=timedelta(minutes=1))
> + assert len(result) == 1
> + assert result[0] == EXAMPLE_ISSUE
> +
> +def test_get_valids_all(http_utils: HttpUtils, httpserver: HTTPServer):
>
> This return an empty list because the sample filename you used don't have
> and valid info. If the files were structured list this:
>
> 'MRMS_MergedReflectivityQC_00.50_{issue.year:04d}{issue.month:02d}{issue.day:02d}'
>
> '-{issue.hour:02d}{issue.minute:02d}{issue.second:02d}'_{valid.year:04d}{valid.month:02d}'
>
> '{valid.day:02d}-{valid.hour:02d}{valid.minute:02d}{valid.second:02d}.grib2.gz'
> It should return a valid datetime that matches the issue. We want
> http_utils to be generic enough to handle more than MRMS so we need to test
> that it can get valids.
> ------------------------------
>
> In python/idsse_common/test/test_http_utils.py
> <#85 (comment)>
> :
>
> > +def test_get_issues_with_same_start_stop(http_utils: HttpUtils, httpserver: HTTPServer):
> + url = '/data/'+EXAMPLE_ENDPOINT+'/'
> + httpserver.expect_request(url).respond_with_data(EXAMPLE_RETURN, content_type="text/plain")
> + result = http_utils.get_issues(issue_start=EXAMPLE_ISSUE, issue_end=EXAMPLE_ISSUE, time_delta=timedelta(minutes=1))
> + assert len(result) == 1
> + assert result[0] == EXAMPLE_ISSUE
> +
> +def test_get_valids_all(http_utils: HttpUtils, httpserver: HTTPServer):
> + url = '/data/'+EXAMPLE_ENDPOINT+'/'
> + httpserver.expect_request(url).respond_with_data(EXAMPLE_RETURN, content_type="text/plain")
> + result = http_utils.get_valids(EXAMPLE_ISSUE)
> + assert len(result) == 0
> + #assert result[0] == (EXAMPLE_VALID, f'{EXAMPLE_URL}{EXAMPLE_PROD_DIR}{EXAMPLE_FILES[1]}')
> +
> +
> +def test_get_valids_with_wildcards(http_utils_with_wild: HttpUtils, httpserver: HTTPServer):
>
> Again if the filepath format contained valid this should return
> something. If you want, I'd be okay if we dropped this test unless it is
> need for code coverage. If you do drop it make sure to remove
> http_utils_with_wild feature if it isn't used anywhere else.
>
> —
> Reply to this email directly, view it on GitHub
> <#85 (review)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ATO5PJLTF6ROC6NHIOA2JH32D5YDHAVCNFSM6AAAAABRBCNJVOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDINZZHA4TGOJYGU>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
--
Paul Hamer
<http://www.esrl.noaa.gov/fiqas/>
Global Systems Laboratory (GSL)
325 Broadway R/GS5
Boulder, CO 80305-3328
--
Paul Hamer
<http://www.esrl.noaa.gov/fiqas/>
Global Systems Laboratory (GSL)
325 Broadway R/GS5
Boulder, CO 80305-3328
|
…rror for invalid filepaths...
Geary-Layne
reviewed
Dec 12, 2024
|
For observation datasets there are no "valids" as defined in the
"file_base" configuration,
ie {valid.year:04d}{valid.month:02d}{valid.day:02d}... etc
The base class handles this as expected for the MRMS examples and for NBM,
if anything the test for 0 is correct in this example set.
…On Thu, Dec 12, 2024 at 3:08 PM Geary Layne ***@***.***> wrote:
***@***.**** commented on this pull request.
I still see two changes that needs to be made. 1) Remove module level
pylint disable (line 23) from http_utils.py, and 2) change the
test_get_valid... so that they return something (not empty lists) in
test_http_utils.py.
------------------------------
In python/idsse_common/idsse/common/http_utils.py
<#85 (comment)>
:
> +# Contributors:
+# Paul Hamer (1)
+#
+# -------------------------------------------------------------------------------
+import logging
+import os
+import shutil
+from collections.abc import Sequence
+
+import requests
+
+from .protocol_utils import ProtocolUtils
+
+logger = logging.getLogger(__name__)
+
+# pylint: disable=broad-exception-caught
You have this below at the actual exception
⬇️ Suggested change
-# pylint: disable=broad-exception-caught
—
Reply to this email directly, view it on GitHub
<#85 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ATO5PJMAYUHFC5TE4QKVH7L2FICOJAVCNFSM6AAAAABRBCNJVOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIOBVHAYTKMRSG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Paul Hamer
<http://www.esrl.noaa.gov/fiqas/>
Global Systems Laboratory (GSL)
325 Broadway R/GS5
Boulder, CO 80305-3328
|
Geary-Layne
approved these changes
Dec 13, 2024
There was a problem hiding this comment.
I think this is good to merge now.
Creating mock file names with both issue and valid was a good choice. I don't understand how wildcards work, but didn't with old aws_utils. Maybe take a look because the mock wildcard instance looks strange to me with the '?' between minute and second.
@fixture
def http_utils_with_wild() -> HttpUtils:
EXAMPLE_BASE_DIR = 'http://127.0.0.1:5000/data/'
EXAMPLE_SUB_DIR = '3DRefl/MergedReflectivityQC_00.50/'
EXAMPLE_FILE_BASE = ('MRMS_MergedReflectivityQC_00.50_{issue.year:04d}{issue.month:02d}{issue.day:02d}'
'-{issue.hour:02d}{issue.minute:02d}?{issue.second:02d}')
EXAMPLE_FILE_EXT = '.grib2.gz'
|
I'll check this...
…On Fri, Dec 13, 2024 at 9:32 AM Geary Layne ***@***.***> wrote:
***@***.**** approved this pull request.
I think this is good to merge now.
Creating mock file names with both issue and valid was a good choice. I
don't understand how wildcards work, but didn't with old aws_utils. Maybe
take a look because the mock wildcard instance looks strange to me with the
'?' between minute and second.
@fixture
def http_utils_with_wild() -> HttpUtils:
EXAMPLE_BASE_DIR = 'http://127.0.0.1:5000/data/'
EXAMPLE_SUB_DIR = '3DRefl/MergedReflectivityQC_00.50/'
EXAMPLE_FILE_BASE = ('MRMS_MergedReflectivityQC_00.50_{issue.year:04d}{issue.month:02d}{issue.day:02d}'
'-{issue.hour:02d}{issue.minute:02d}?{issue.second:02d}')
EXAMPLE_FILE_EXT = '.grib2.gz'
—
Reply to this email directly, view it on GitHub
<#85 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ATO5PJNSFACPNBTLYZFTRSD2FMD3FAVCNFSM6AAAAABRBCNJVOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMBSGY4DQMJWHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Paul Hamer
<http://www.esrl.noaa.gov/fiqas/>
Global Systems Laboratory (GSL)
325 Broadway R/GS5
Boulder, CO 80305-3328
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Linear Issue
IDSSE-1001
Changes
Explanation
Required supporting code for MRMS observation data ingest from NSSL directly