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

Automatically find EWAH files with increased index_order2 #4198

Merged
merged 6 commits into from
Nov 18, 2022

Conversation

jzuhone
Copy link
Contributor

@jzuhone jzuhone commented Nov 7, 2022

PR Summary

When loading particle datasets, a morton index is either created and stored to an EWAH file or is loaded from an already existing file.

PR #3198 allowed for the automatic updating of the refined index to be more efficient. However, since both the coarse and the fine index are in the filename, an existing EWAH file with the updated index order is not found and thus the index must be re-generated again.

This PR changes the logic slightly so that EWAH filenames with either the original refined index or the updated one can be found and used.

Closes Issue #3487.

PR Checklist

  • New features are documented, with docstrings and narrative docs
  • Adds a test for any bugs fixed. Adds tests for new features.

@neutrinoceros
Copy link
Member

about the CodeTour watch failure: it might be worth trying bumping this action (current latest is 1.6.3 https://github.com/pozil/codetour-watch/releases)

@neutrinoceros neutrinoceros added this to the 4.1.2 milestone Nov 7, 2022
@matthewturk
Copy link
Member

@jzuhone this is awesome and I think will make a lot of lives easier. (cc @chummels )

Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

I like the idea, and I assume this is hard to test automatically so I guess it's fine as is, provided it can at least be manually validated.

Co-authored-by: Clément Robert <cr52@protonmail.com>
@jzuhone
Copy link
Contributor Author

jzuhone commented Nov 8, 2022

@neutrinoceros thanks for the catch on range. I did do some manual testing as well.

@neutrinoceros
Copy link
Member

Note that the fix to CodeTour failures is to update https://github.com/yt-project/yt/blob/main/.tours/particle-indexing.tour to match the new line numbers

@jzuhone
Copy link
Contributor Author

jzuhone commented Nov 8, 2022

Welp, I updated the line numbers in codetour and updated the hash for the commit it should use and we're still failing

@matthewturk
Copy link
Member

@jzuhone I'll take a look

@neutrinoceros
Copy link
Member

I tried just bumping the action, to no avail. I don't understand why it's still failing.

@matthewturk
Copy link
Member

matthewturk commented Nov 9, 2022 via email

@jzuhone
Copy link
Contributor Author

jzuhone commented Nov 10, 2022

Some googling of the error message it prints out ("Resource not accessible by integration") suggests some kind of weird permissions problem

@jzuhone
Copy link
Contributor Author

jzuhone commented Nov 16, 2022

The test failure here has something to do with a GitHub secret that apparently has a permissions issue. Not sure if I can say much more than that.

@jzuhone
Copy link
Contributor Author

jzuhone commented Nov 16, 2022

Is it because I don't have the secret in my fork? I'm not sure why it would fail only on this PR then

@neutrinoceros
Copy link
Member

I don't think the problem is with your fork, but with the main repo's configuration (maybe the secret's name is incorrect ?)
We absolutely need a repo owner to look at this, but it seems to me that this workflow never worked properly (apparently it's supposed to write automated comments in the discussion thread).
@matthewturk , if you're not able to fix this soon, I think we should just turn off this workflow for now rather than blocking the next bugfix release over this

@Xarthisius
Copy link
Member

Xarthisius commented Nov 17, 2022

Looking at what it want to paste as a comment:

CodeTour Watch

Changed files with possible CodeTour impact:

  • yt/geometry/particle_geometry_handler.py

Impacted CodeTour files:

  • .tours/particle-indexing.tour

Make sure to review CodeTour files and update line numbers accordingly.

Maybe if we do that it will go away... :) Oh, I see you did that already.

@Xarthisius
Copy link
Member

I checked that GITHUB_TOKEN is enabled, and has write permission to issues (everything really). I don't know why it's failing...

@jzuhone
Copy link
Contributor Author

jzuhone commented Nov 17, 2022

I filed an issue with the action maintainer

@neutrinoceros
Copy link
Member

Thank you for taking a look. I can open a PR to propose we deactivate this workflow as well as an issue to keep track of this, but I won't be able to before a couple hours (if anyone wants to do it before that, feel free to !)

@neutrinoceros
Copy link
Member

issue: #4213
and PR: #4214

@neutrinoceros
Copy link
Member

Just so this remains 100% relevant to backport, I'll remove my vain commit

@neutrinoceros neutrinoceros merged commit b567939 into yt-project:main Nov 18, 2022
meeseeksmachine pushed a commit to meeseeksmachine/yt that referenced this pull request Nov 18, 2022
neutrinoceros added a commit that referenced this pull request Nov 18, 2022
…8-on-yt-4.1.x

Backport PR #4198 on branch yt-4.1.x (Automatically find EWAH files with increased index_order2)
neutrinoceros added a commit that referenced this pull request Dec 15, 2022
matthewturk added a commit that referenced this pull request Dec 16, 2022
…-pr-4198-on-yt-4.1.x

Revert "Backport PR #4198 on branch yt-4.1.x (Automatically find EWAH files with increased index_order2)"
@jzuhone jzuhone deleted the fix_ewah branch April 10, 2023 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants