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

Allow rethrowing of error from @require #63

Closed
wants to merge 1 commit into from

Conversation

jkozdon
Copy link

@jkozdon jkozdon commented Apr 5, 2019

This has been mentioned in #16, but I was wondering how you would feel about the following interface for allowing @requires to rethrow error messages.

@require Logging="56ddb016-857b-54e1-b83d-db4d58db5568" rethrow=true include("foo.jl")
@require Logging="56ddb016-857b-54e1-b83d-db4d58db5568" rethrow=false include("foo.jl")
@require Logging="56ddb016-857b-54e1-b83d-db4d58db5568" include("foo.jl")
``
The first case would result in the an error being propagated in the case that "foo.jl" is not defined whereas the other two cases would be the behavior as it currently is.

This came up in our project because something passed our CI even with a requires warning.

Still need to add tests which I can do if you are open to the idea.

@MikeInnes
Copy link
Collaborator

This is fine if you do using Logging; using MyPackage since things will error when MyPackage is loaded. But if you do using MyPackage; using Logging you'd get an error loading the Logging package, which is confusing since that's not where the issue is. Hence the current warning.

One option might be to have an environment variable like REQUIRES_THROW which you can just turn on during CI.

(Aside: it's a bit odd to need this, since if you're happy to have something as a test dependency it seems like it'd be light enough for a regular dependency. But don't let me tell you how to live your life.)

@vchuravy
Copy link
Member

vchuravy commented Apr 5, 2019

(Aside: it's a bit odd to need this, since if you're happy to have something as a test dependency it seems like it'd be light enough for a regular dependency. But don't let me tell you how to live your life.)

Well it's conditional loading of GPU code on a GPU CI.

@jkozdon
Copy link
Author

jkozdon commented Apr 5, 2019

Agreed that it's a bit weird. As @vchuravy pointed out, the issue was with GPU CI, and if we had more complete GPU CI the error would have popped elsewhere.

We're using Requires.jl to have optional dependencies in our code. So if, for instance, CuArrays is loaded then we know to include the CUDA kernels and functionality. Otherwise, we're running on a non-cuda system so we cannot load the CUDA packages. Since Julia doesn't have a way of specifying optional dependencies in the Project and Manifest, this is our work around (and you can only include the CUDA packages on systems that have NVIDIA GPUs).

We'll be doing a similar thing for MPI soon as well.

@MikeInnes
Copy link
Collaborator

Ah, I see – yeah, that does make sense then.

@jkozdon jkozdon closed this Aug 23, 2019
@jkozdon
Copy link
Author

jkozdon commented Aug 23, 2019

Decided this wasn't needed after all, but forgot to close the PR

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.

3 participants