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

Fix margin cache generation leaving empty shard dirs #205

Closed
wants to merge 2 commits into from

Conversation

smcguire-cmu
Copy link
Contributor

@smcguire-cmu smcguire-cmu commented Jan 30, 2024

Change Description

Fixes a bug where generating a margin cache left the empty top level shard directories.

Solution Description

In a previous PR, the margin cache shards were changed to be generated from the final pixel destination Norder=x/Dir=y/Npix=z/... to the same shard directories as other pipelines order_x/dir_y/pix_z/.... Before, these directories did not need to be deleted since they were used as the final structure, but now the new shard directory structures do need to be deleted.

To solve this, the shard files are now stored under a subdirectory cache/order_x/... which is deleted at the end of the pipeline.

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

Copy link

codecov bot commented Jan 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (862ea03) 99.74% compared to head (b082078) 99.74%.

❗ Current head b082078 differs from pull request most recent head a641c79. Consider uploading reports for the commit a641c79 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #205   +/-   ##
=======================================
  Coverage   99.74%   99.74%           
=======================================
  Files          24       24           
  Lines        1166     1172    +6     
=======================================
+ Hits         1163     1169    +6     
  Misses          3        3           

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

file_io.remove_directory(args.tmp_path, ignore_errors=True)
file_io.remove_directory(cache_path, ignore_errors=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm. It looks like the margin cache process doesn't make use of the args.tmp_path at all, and would have been cleaned up by the above step, if it were. I created a branch that uses the tmp_path instead (https://github.com/astronomy-commons/hipscat-import/tree/delucchi/margin_tmp). Can you see if that addresses the empty shard dir problem? I'd like users to be able to use a different path for tmp files (see this doc: https://hipscat-import.readthedocs.io/en/latest/catalogs/temp_files.html)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see! That makes sense, and yeah I tested with that branch and it solves the issue. I can close this PR and we can go with that solution if you think that's better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. Sending you the PR.

@smcguire-cmu
Copy link
Contributor Author

#207 fixes the same issue as this and uses the tmp_path for intermediate files, so no need for this.

@delucchi-cmu delucchi-cmu deleted the sean/fix-margin-cache-temp-dirs branch April 2, 2024 13:06
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