Skip to content

Conversation

@davidhassell
Copy link
Collaborator

Fixes #306

@davidhassell
Copy link
Collaborator Author

The cfa changes (just better handling the traceback from exceptions) were an accidental spill over, but they might as well get reviewed here!

@sadielbartholomew
Copy link
Member

Hi @davidhassell, the CI jobs look like they are hanging here (over 4 hours runtime) so I suggest cancelling them. I have seen these tests hanging recently and need to investigate that, and given that a few have passed enough for us to be confident this is good along with a local full-suite pass, it seems highly unlikely they are hanging due to this PR. So I'll ignore the CI in my reviewing. Thanks.

@davidhassell
Copy link
Collaborator Author

Hi Sadie, the CI jobs had finally dies by the time a looked )or perhaps you killed them?). I must confess to usually running the tests individually these days, as I still get sporadic hanging - and they passed in this way for me on this PR. Do you think that moving to pytest might be helpful in solving this issue? I don't have any evidence for this, other than it's different to unittest! It's a bigger job, but something that could be nice in the longer term. That's a discussion for elsewhere, anyway.

Copy link
Member

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

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

One minor issue and a question reported in-line, but overall this is good to merge: fixes the issue at hand in the most sensible way and the tests pass locally, including the suitable new unit test.

Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
@sadielbartholomew
Copy link
Member

I must confess to usually running the tests individually these days, as I still get sporadic hanging - and they passed in this way for me on this PR.

Ah, sorry about this and the inconvenience. Something is up and it may just be in the Actions infrastructure. Once the machine and performance release is out, maybe I can briefly investigate.

Do you think that moving to pytest might be helpful in solving this issue? I don't have any evidence for this, other than it's different to unittest! It's a bigger job, but something that could be nice in the longer term. That's a discussion for elsewhere, anyway.

I think that is much more likely to be about some specific issue(s) with the codebase or underlying C libraries rather than the test framework used, but I think moving to pytest would be a good idea anyway, e.g. we can parametrise the test methods very easily and cleanly, as you are probably aware. So definitely keep that flagged for the future (in your mind or you could even open up an Issue if one isn't already there).

@sadielbartholomew
Copy link
Member

This is good to go so I will merge, just to tie up loose ends before the weekend...

@sadielbartholomew sadielbartholomew merged commit 6b20f77 into NCAS-CMS:master Feb 4, 2022
@davidhassell davidhassell added this to the 3.13.0 milestone Jun 23, 2022
@davidhassell davidhassell deleted the umread-1 branch November 15, 2022 09:12
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.

UM file opening: error message is unfriendly if "um" arg to "cf.read" is missing required keys

2 participants