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

Add verification tests #379

Closed
wants to merge 6 commits into from

Conversation

troyraen
Copy link
Collaborator

@troyraen troyraen commented Aug 14, 2024

Quick-start for anyone wanting to test this code

Code in this PR is in flux. I will update this comment as necessary when I change code. Currently (18 Oct 2024) this works only with HiPSCat (not HATS) catalogs.

import hipscat_import.verification.run_verification as runner
from hipscat_import.verification.arguments import VerificationArguments

# Change this to point at the HiPSCat catalog's root directory.
# If you have this branch checked out and are in the repo root,
# small_sky_object_catalog can be used for a quick test.
input_catalog_path = "tests/hipscat_import/data/small_sky_object_catalog"
# Directory where you want verification reports written
output_path = "verification_output"
# If you have a parquet schema you want to use as "truth", point this at that file
truth_schema = None
# If you know how many rows SHOULD be in the catalog, enter that number here
truth_total_rows = None

args = VerificationArguments(
    output_path=output_path,
    input_catalog_path=input_catalog_path,
    truth_total_rows=truth_total_rows,
    truth_schema=truth_schema,
)

# Next line will run all verification tests and write a report
verifier = runner.run(args, write_mode="w")
# Look at the results (same content as the written report)
verifier.results_df

# If you prefer to run tests individually, do this:
verifier = runner.Verifier.from_args(args)
verifier.test_file_sets()
verifier.test_is_valid_catalog()
verifier.test_num_rows()
verifier.test_rowgroup_stats()
verifier.test_schemas()

Change Description

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

Closes #118, closes #373, closes #374

Adds the following:

  • Verifier class
    • verification tests: hipscat is_valid_catalog, file sets (_metadata vs files on disk), row counts, row group statistics, schemas
    • output files: verifier_results.csv, field_distributions.csv
  • Test data for malformed catalogs
    • datasets: bad_schemas, no_rowgroup_stats, wrong_files_and_rows
    • code used to generate the datasets: generate_malformed_catalogs.py
  • Pytest fixtures
    • VerifierFixture class and related configs file fixture_defs.yaml. These define the Verifier instances to be used in unit tests and their expected outcomes. I set it up this way because the grid of options to be unit tested is large.
  • Unit tests for each Verifier test.

Stil to-do:

  • Connect to provenance info
  • Check code style
  • Determine whether there is overlap with existing hipscat validation and/or whether any of this code should be moved to hipscat repo.
  • Test a large catalog and determine whether tests may benefit from parallelization with dask.

Code Quality

  • I have read the Contribution Guide and LINCC Frameworks Code of Conduct
  • 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

Project-Specific Pull Request Checklists

New Feature Checklist

  • I have added or updated the docstrings associated with my feature using the NumPy docstring format
  • I have updated the tutorial to highlight my new feature (if appropriate)
  • I have added unit/End-to-End (E2E) test cases to cover my new feature
  • My change includes a breaking change
    • My change includes backwards compatibility and deprecation warnings (if possible)

Copy link

codecov bot commented Aug 14, 2024

Codecov Report

Attention: Patch coverage is 29.62963% with 19 lines in your changes missing coverage. Please review.

Project coverage is 98.47%. Comparing base (e7f9b4c) to head (029196d).

Files Patch % Lines
...rc/hipscat_import/verification/run_verification.py 29.62% 19 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #379      +/-   ##
==========================================
- Coverage   99.72%   98.47%   -1.26%     
==========================================
  Files          26       26              
  Lines        1481     1508      +27     
==========================================
+ Hits         1477     1485       +8     
- Misses          4       23      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@troyraen
Copy link
Collaborator Author

@delucchi-cmu here are some options. Take a look and let me know what you think. It's obviously not fully integrated yet. I did add some data that will make the tests fail; not sure if you want those extra files or if it should somehow use data that's already here. Also, it probably needs to use your custom file pointers and the user-supplied storage kwargs that you support, but I haven't added them yet. Also haven't done docstrings yet (these probably aren't the functions you actually want anyway).

There are four tests. The one checking the file schemas against _common_metadata is the thing you actually asked for. It feels a little incomplete if the _common_metadata isn't checked against a schema from the user, but if you don't want to rely on user input here I can understand that. The other tests are code that I had handy that's inline with checking individual parquet files. One is row counts, which I know you're taking care of in a different repo but I think you said that didn't check the individual files? In any case, I'm happy to take all those extra three tests out if you just want to focus on the schema right now.

I'm not sure how you want this integrated with the main run function. Right now, the functions I wrote return a boolean indicating pass/fail. What do you want it to do if it fails? I'm guessing it should not raise an error. Should it just print messages to std out, or create a report file, or ..?

Copy link

github-actions bot commented Sep 13, 2024

Before [829fe47] After [27dad80] Ratio Benchmark (Parameter)
failed failed n/a benchmarks.BinningSuite.time_read_histogram

Click here to view all benchmarks.

@troyraen troyraen changed the title Raen/verify/files Add verification tests Sep 18, 2024
@troyraen troyraen self-assigned this Sep 18, 2024
@troyraen
Copy link
Collaborator Author

I made choices about some questions from above and filled out this code a little more. So right now there is a Verifier class that handles the tests and it's called in the run function with:

verifier = Verifier.from_args(args)
verifier.test_is_valid_catalog()  # run hipscat.io.validation.is_valid_catalog
verifier.test_schemas()  # user-provided schema vs _common_metadata, _metadata, and file footers
verifier.test_num_rows()  # file footers vs _metadata (per file), and user-provided total
verifier.record_results()  # write a verification report

verifier.record_distributions()  # calculate distributions (min/max of all fields) and write a file

@troyraen troyraen marked this pull request as ready for review October 16, 2024 16:03
@troyraen
Copy link
Collaborator Author

troyraen commented Nov 5, 2024

superseded by #428

@troyraen troyraen closed this Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant