Skip to content
This repository was archived by the owner on Nov 17, 2025. It is now read-only.

Conversation

@dfm
Copy link
Contributor

@dfm dfm commented Oct 7, 2020

I believe that this will fix #87.

I'm finding that some of the jax tests are failing on my fork though:

tests/sandbox/test_jax.py::test_extra_ops fails with:

jax.core.ConcretizationTypeError: Abstract tracer value encountered where concrete value is expected.

@dfm
Copy link
Contributor Author

dfm commented Oct 7, 2020

Also: I that the only way to totally fix #87 will be to make the coverage check not required.

@dfm dfm marked this pull request as draft October 7, 2020 15:41
@brandonwillard
Copy link
Member

Also: I that the only way to totally fix #87 will be to make the coverage check not required.

The latter part of #87? If so, I'm fine dealing with the uncompleted checks if fixing it means disabling the coverage requirement, because we're really gonna need to enforce the coverage checks as development starts ramping up.

@dfm dfm marked this pull request as ready for review October 7, 2020 17:42
@brandonwillard
Copy link
Member

I'm finding that some of the jax tests are failing on my fork though:

Hmm, not good; I'll check that out.

@dfm
Copy link
Contributor Author

dfm commented Oct 7, 2020

Yeah - that's what I meant! I don't think that we can have the coveralls report be a required check if we're not going to run the tests for some pull requests because coverage won't be reported for those cases. It'll still show up as a failure on the pull request, but merging won't be blocked.

This PR is ready for review now, I think. It adds an "All tests" check that reports the results of all the matrix builds. Once the action finishes running, we can make that the required check.

The jax test is still failing, but this pull request can't have caused any of those so I'm a bit confused.

@brandonwillard
Copy link
Member

The jax test is still failing, but this pull request can't have caused any of those so I'm a bit confused.

My guess is that it has to do with some (effectively) random test values.

@brandonwillard
Copy link
Member

Nope, I was wrong, it's due to a recent jax/jaxlib update!

@brandonwillard
Copy link
Member

Relevant issue: #89

@brandonwillard
Copy link
Member

Are we uploading the coverage for every matrix job?

@dfm
Copy link
Contributor Author

dfm commented Oct 8, 2020

Yes, but those don't get displayed on coveralls until the final job where coveralls is told to "finish" and combine all the steps. This is the recommended procedure for coveralls on actions.

@dfm dfm marked this pull request as draft October 8, 2020 13:57
@dfm dfm marked this pull request as ready for review October 8, 2020 18:26
@dfm
Copy link
Contributor Author

dfm commented Oct 8, 2020

@brandonwillard: In response to your implied concern I switched to merging the coverage results in place and then uploading them once. That way the "checks" tab doesn't get spammed. It looks like it worked so this should be good to go.

@brandonwillard
Copy link
Member

I switched to merging the coverage results in place

Many thanks! It doesn't appear to be a real problem, but it is a bit jarring in some ways.

@brandonwillard brandonwillard merged commit d37c620 into aesara-devs:master Oct 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Simplify/combine status checks

2 participants