-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add MOM6 support (om4 025jra ryf) #258
base: main
Are you sure you want to change the base?
Conversation
…tamp' groups in filename regexp
…list of xfailing tests
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #258 +/- ##
==========================================
- Coverage 97.90% 97.36% -0.54%
==========================================
Files 11 11
Lines 810 836 +26
==========================================
+ Hits 793 814 +21
- Misses 17 22 +5 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
All tests passing - just codecov that's not passing. I guess that must mean that CI environment isn't mirroring Gadi correctly.. |
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.
Couple of minor comments & a bunch of empty files I think got accidentally committed.
Otherwise looks good - the only thing I think that might warrant some extra thought is the EmptyDataError ( inmanager.py
) - I've left a comment here, I'm not sure that EmptyDataError is the most appropriate?
columns_with_iterables=COLUMNS_WITH_ITERABLES, | ||
) | ||
except EmptyDataError as e: | ||
raise EmptyDataError(str(e) + f": {self.path}") |
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.
Perhaps an issue for intake-dataframe-catalog rather than here, but I feel like we might want to emit a DfFileCatalogError here rather than than an EmptyDataError?
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 think that is an issue for intake-dataframe-catalog
, as you suggested. All I was trying to do is re-emit the same error with a slightly more useful message.
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've opened an issue there. Could you update the catch to
except (EmptyDataError, DfFileCatalogError) as e
so it won't break when we update it there?
return ncinfo_dict | ||
|
||
except Exception: | ||
return {INVALID_ASSET: file, TRACEBACK: traceback.format_exc()} |
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.
Codecov is complaining that this line isn't tested - tbh I think it's unimportant.
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.
Manifest files all empty - probably an accidental git add .
instead of git add --update
?
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'll need to check on Gadi tomorrow - I tried to ape the 'real' structure there as much as possible, and there may be empty manifest yamls on there. Whether it's necessary for testing or not is another matter.
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 think there's a tool I've used which detects unused test data... I'll see if I can dig it out. I think it would be good to keep unused data out as far as is possible.
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.
OK, let me know if you find the tool. The aim was to give the build system access to 'furphy' files to make sure they weren't accidentally ingested as real data (c.f. the access-om3
test data directory).
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 think this got accidentally committed?
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.
See above comment re: duplicating the file structure on Gadi.
Closes #175 .
This PR adds the data requested from #175 , which required a new builder: MOM6Builder.
PR includes relevant builder, translator, and tests.