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

Verification pipeline boilerplate #126

Merged
merged 4 commits into from
Oct 2, 2023
Merged

Conversation

delucchi-cmu
Copy link
Contributor

Change Description

  • My PR includes a link to the issue that I am addressing

Adds the boilerplate for running the new verification pipeline within the existing hipscat-import pipeline infrastructure.

This allows us to inherit:

  • dask client creation
  • success/failure email
  • creation and management of output path and temporary path
  • basic provenance tracking

Code Quality

  • I have read the Contribution Guide
  • My code follows the code style of this project
  • My code builds (or compiles) cleanly without any errors or warnings
  • My code contains relevant comments and necessary documentation

New Feature Checklist

@codecov
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

Merging #126 (883e7d3) into main (1525da2) will not change coverage.
Report is 14 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #126   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        24    +2     
  Lines          900       939   +39     
=========================================
+ Hits           900       939   +39     
Files Coverage Δ
src/hipscat_import/catalog/resume_plan.py 100.00% <100.00%> (ø)
src/hipscat_import/catalog/run_import.py 100.00% <100.00%> (ø)
src/hipscat_import/index/run_index.py 100.00% <100.00%> (ø)
src/hipscat_import/pipeline_resume_plan.py 100.00% <100.00%> (ø)
src/hipscat_import/soap/resume_plan.py 100.00% <100.00%> (ø)
src/hipscat_import/soap/run_soap.py 100.00% <100.00%> (ø)
src/hipscat_import/verification/arguments.py 100.00% <100.00%> (ø)
...rc/hipscat_import/verification/run_verification.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@troyraen troyraen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm out of the office unexpectedly, but plan to be back this Thursday, Sept 7. I can take a closer look when I return. In the meantime, can you give me some direction about what kind of review you'd like? I gather that you'd like me to write the code that run_verification.run will execute? If so, I can look at this PR from the perspective of understanding how to do that. Are there other things you'd like me to look at?

@delucchi-cmu
Copy link
Contributor Author

Our general guidelines and philosophy on code reviews are in this wiki entry: https://github.com/lincc-frameworks/docs/wiki/Design-and-Code-Review-Policy

I'm hoping that by getting this boilerplate PR out of the way, adding the meaningful implementation of the verification logic will be more straightforward. Your assumption of the perspective I'm looking for is correct, though I'm planning to also contribute some to the logic in run_verification - you won't be out on your own =]

@troyraen troyraen self-requested a review September 7, 2023 19:22
Copy link
Collaborator

@troyraen troyraen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions and comments below, but overall LGTM and I'll approve and let you decide when you're ready to merge.

delucchi-cmu and others added 3 commits September 28, 2023 10:42
* Use minimum stage name formatting.

* Run copier.

* Add a tad more context for failure email.

* Pin pandas version

* Remove benchmarks for now.

* unpin sphinx versions (#134)

---------

Co-authored-by: Max West <110124344+maxwest-uw@users.noreply.github.com>
@delucchi-cmu delucchi-cmu merged commit c6f7166 into main Oct 2, 2023
13 checks passed
@delucchi-cmu delucchi-cmu deleted the issue/118/boilerplate branch October 2, 2023 13:17
delucchi-cmu added a commit that referenced this pull request Oct 6, 2023
* unpin sphinx versions (#134)

* Verification pipeline boilerplate (#126)

* Verification pipeline boilerplate

* Input catalog options.

* Contribution docs updates.

* Merge recent changes (#135)

* Use minimum stage name formatting.

* Run copier.

* Add a tad more context for failure email.

* Pin pandas version

* Remove benchmarks for now.

* unpin sphinx versions (#134)

---------

Co-authored-by: Max West <110124344+maxwest-uw@users.noreply.github.com>

---------

Co-authored-by: Max West <110124344+maxwest-uw@users.noreply.github.com>

* Check for valid catalog directories. (#136)

* Check for valid catalog directories.

* Hit uncovered file check.

---------

Co-authored-by: Max West <110124344+maxwest-uw@users.noreply.github.com>
@maxwest-uw maxwest-uw mentioned this pull request Oct 25, 2023
10 tasks
@delucchi-cmu delucchi-cmu mentioned this pull request Nov 21, 2023
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.

2 participants