Adding files with missing tree to bad files #730
Conversation
|
@MoAly98 you need to fix the flake8 and black errors in this PR. :-) |
|
|
hopefully this fixes them. Should I rebase as well since you released new coffea? |
|
You've got a few problems to fix. You can test them on your local computer using pytest: |
|
Weird to fail. I ran into the serviceX issue locally but I don't think I introduced it. I also had not ran into the others locally. |
MoAly98
force-pushed
the
maly-skipbadfiles-missing-trees
branch
from
eef9ff5
to
6762a25
Compare
|
@MoAly98 the workqueue tests fail in this case - can you please investigate? Thanks! |
urgh yeah i know what's wrong.. forgot we are dealing with generators here. I don't think its so expensive to check with |
|
Should be fine so long as it doesn't cause other regressions. |
…tor/list depending on meta
|
local tests pass :) |
|
I preferred a |
|
can you force push this or something to make the CI turn over - it's weird it didn't catch it.. |
lgray
changed the title
|
@MoAly98 This is still broken for workqueue - please see the latest CI run. Please fix this. |
|
Perhaps also - @btovar as well for a second set of eyes. |
|
I am actually not so sure. What is the work queue doing that is not being checked by the test suite? also how do I test the work-queue locally? |
| if wrapped_out is None: | ||
| def check_chunks(x): | ||
| if meta: | ||
| return next(x, None) is not None |
There was a problem hiding this comment.
I think this might be a problem. The first element of the generator will be lost?
There was a problem hiding this comment.
I wonder if it is better to simply check the len() == 0 of chunks_to_count above (line 1811). It leaves chunks alone if it comes from a generator, and it has the same contents as chunks if not. Further, you may not need to define the function check_chunks anymore.
There was a problem hiding this comment.
Yes thats a much better idea. It also separates the lack of chunks from checking bad processors. Thanks! I implemented and pytest passes at least.
There was a problem hiding this comment.
If the first element is lost (which it should, you're right) the tests should fail ... makes me think the test is not covering something.
There was a problem hiding this comment.
This is what's breaking. You can still get chunks_to_count/chunks as a generator. I guess a check_chunks functio there could make sense...
|
@lgray Did you intend this one to be merged? |
You should be able to |
it is complaining that |
|
For some reason the CI broke in the PR and I couldn't get it to run again. If we can't fix this by thursday I'll revert the merge. |
How did you install work queue? Usually it comes from a conda install, like # Create a new environment
cd coffea
conda create --yes --name coffea-env -c conda-forge python ndcctools conda-pack
conda activate coffea-env
# install coffea in edit mode, so that changes get immediately available:
python -m pip install --ignore-installed .
cd tests
python wq.py |
|
@lgray @btovar I got rid of the chunks length check all together -- I think it was actually redundant and something I had introduced to avoid a test breaking but the problem was originally in the test itself so the line was not needed at all. I have now updated the exectutor module, and I also changed the tests around so that they check for cases where a tree is present in one file but not the other, as well as when the tree is missing from both files. The tests all pass locally, and linting also passes. I also ran the WQ and it seems to work fine. On that note though, I wonder why the WorkQueue test is not as extensive as the rest of the executors? Does running a coverage really show that all WorkQueue branchings are covered? |
|
@MoAly98 I think it is because the wq executor needs to be setup with conda, so we have not got around to add it to the rest of the python tests. |
|
@MoAly98 can you submit a new pr with the changes? |
No description provided.