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

Support additional variable in PathBuilder #90

Merged
merged 14 commits into from
Dec 6, 2024

Conversation

Geary-Layne
Copy link
Contributor

Linear Issue

IDSSE-836

Changes

  • Previously PathBuilder only support issue/valid/lead, and now we can support arbitrary variables (like region)
  • There is also a change to the underlying algorithm
    • Previously, every time a path string was to be parsed, the format string was reevaluated
    • Now, the format string are evaluated when provided to the class and store to a dictionary for reuse without reevaluation
  • A couple public static methods were made private
    • The same info is available via simple getting methods
    • And since they are not private, the direct unit tests were removed
  • The TimeDelta class was switch from wrapping a timedelta object to extending it. Note TimeDelta is needed to all {lead.hour} to be used in path formats

Explanation

There are a few mostly unrelated changes to other class, but they are all small

mackenzie-grimes-noaa and others added 3 commits December 5, 2024 07:18
* add protections for Publisher/Consumer using default exch or queue
* setting prefetch_count must be done before calling basic_consume
* re-order rabbitmq_utils classes and functions
---------

Co-authored-by: Geary Layne <[email protected]>
@paulhamer-noaa
Copy link
Contributor

Do these changes include the parse_args update from PR #85? I didn't see anything in the changed code that included catching the exception to calling this in parse_times with a malformed path?

@Geary-Layne
Copy link
Contributor Author

Paul, good call. I forgot that bug fix, I make sure it's covered

@Geary-Layne
Copy link
Contributor Author

Geary-Layne commented Dec 5, 2024

@paulhamer-noaa, I looked into where you added the try/except. The code with in the 'try' was doing two things, 1) parsing the format string, and 2) extracting the variable from the provided path string. These two step are not separated. Parsing the format string is done on init or format updates, and this info is saved for later when extracting. If you have the example/test that used to break, I can make sure it works with the new code.

@paulhamer-noaa
Copy link
Contributor

Use any unittest for http_utils that use the MRMS return. The mrms_response includes a file with the name:

MRMS_MergedReflectivityQC_00.50.latest.grib2.gz

@Geary-Layne
Copy link
Contributor Author

Geary-Layne commented Dec 5, 2024

Locally I added this temporarily to the test_path_builder.py (with a few changed to path_builder.py)

@fixture
def http_utils() -> PathBuilder:
    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'

    return PathBuilder(EXAMPLE_BASE_DIR, EXAMPLE_SUB_DIR, EXAMPLE_FILE_BASE, EXAMPLE_FILE_EXT)

def test_invalid_path_len(http_utils: PathBuilder):
    path_with_invalid_struct = 'MRMS_MergedReflectivityQC_00.50.latest.grib2.gz'

    http_utils.parse_filename(path_with_invalid_struct)


def test_invalid_path_type(http_utils: PathBuilder):
    path_with_invalid_struct = 'MRMS_MergedReflectivityQC_00.50.abcdEFgh-IJklMN.grib2.gz'

    http_utils.parse_filename(path_with_invalid_struct)

and get this:
FAILED test_path_builder.py::test_invalid_path_len - ValueError: Path is not expected length. Passed path part 'MRMS_MergedReflectivityQC_00.50.latest.grib2.gz' doesn't match format 'MRMS_MergedReflecti...
FAILED test_path_builder.py::test_invalid_path_type - ValueError: Unable to apply formatting: int('abcd')

So the problematic paths are handled, just not silently. I believe the try/except/pass code would not raise a exception to let the use know that there is a problem. Would it work for you to add a try/except/pass to code using path builder if the you need it to be silent? As a note, if path had both valid and invalid structure, I believe the try/except/pass code could act like everything was find. Say for a path like {issue.year:04d}-{issue.month:02d}/{issue.day:02d}-{issue.hour:02d}/MRMS_MergedReflectivityQC_00.50_{issue.year:04d}{issue.month:02d}{issue.day:02d}-{issue.hour:02d}{issue.minute:02d}{issue.second:02d}.grib2.gz

paulhamer-noaa
paulhamer-noaa previously approved these changes Dec 6, 2024
@mackenzie-grimes-noaa mackenzie-grimes-noaa dismissed stale reviews from paulhamer-noaa and themself via 7440926 December 6, 2024 20:30
@mackenzie-grimes-noaa
Copy link
Contributor

Sorry about pushing changes to your branch, Geary. I just caused merge conflicts in rabbitmq_utils.py when I merged #87 into main, so I was trying to resolve those for you.

@Geary-Layne Geary-Layne merged commit 5413279 into main Dec 6, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants