-
Notifications
You must be signed in to change notification settings - Fork 16
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
Updating CLI to fix MAG #404
Updating CLI to fix MAG #404
Conversation
version = f"v{int(dataset.attrs['Data_version']):03d}" # vXXX | ||
r = re.compile(r"v\d{3}") | ||
if r.match(dataset.attrs["Data_version"]) is None: | ||
version = f"v{int(dataset.attrs['Data_version']):03d}" # vXXX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original version was throwing errors because batch starter provides the version as "vXXX"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. can you add this comment in the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
@@ -78,6 +84,11 @@ def process_and_write_data( | |||
if not packet_data: | |||
return [] | |||
|
|||
# TODO: Rework attrs to be better | |||
raw_attrs["Data_version"] = data_version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a temporary fix, I wanted to run L1A in the cloud today for MAG but I have rewritten the way these attributes work in my WIP branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great!
version = f"v{int(dataset.attrs['Data_version']):03d}" # vXXX | ||
r = re.compile(r"v\d{3}") | ||
if r.match(dataset.attrs["Data_version"]) is None: | ||
version = f"v{int(dataset.attrs['Data_version']):03d}" # vXXX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. can you add this comment in the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall.
raw_attrs: dict, | ||
mago_attrs: dict, | ||
magi_attrs: dict, | ||
data_version: str, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is unclear to me which part of these changes is a temporary. If the addition of data_version
to this function signature is not temporary, please add it to the docstring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of it is temporary, including data_version in this change. I need to add a bunch of global attributes so I'm reworking the way this works.
logger.info(f"Uploading file: {filename_burst}") | ||
imap_data_access.upload(filename_burst) | ||
if len(output_files) == 0: | ||
print("No files to upload.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these print
s be logging calls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When they were logging calls, the AWS batch runner wouldn't print them into the cloudwatch logs
imap_processing/cdf/utils.py
Outdated
@@ -66,7 +67,11 @@ def write_cdf(dataset: xr.Dataset): | |||
start_time = np.datetime_as_string(dataset["epoch"].values[0], unit="D").replace( | |||
"-", "" | |||
) | |||
version = f"v{int(dataset.attrs['Data_version']):03d}" # vXXX | |||
r = re.compile(r"v\d{3}") | |||
if r.match(dataset.attrs["Data_version"]) is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r.match
will raise a TypeError if dataset.attrs["Data_version"]
is not a string. Is this a possibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a possibility but I think it would be an incorrect usage of dataset attributes. But, I can add a catch here if it's an int.
b248149
into
IMAP-Science-Operations-Center:dev
Change Summary
Missed this section during my refactor. Just a minor change to remove old filename management.