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

Problematic try exception import block #350

Closed
tunnell opened this issue Jan 28, 2021 · 9 comments
Closed

Problematic try exception import block #350

tunnell opened this issue Jan 28, 2021 · 9 comments
Labels
bug Something isn't working

Comments

@tunnell
Copy link
Contributor

tunnell commented Jan 28, 2021

If utilix isn't installed, it doesn't tell you. However, if utilix fails to load for some reason, then it won't tell you... you can check if Travis through environmental variables, or at least print a warning that there was an exception? You can log exceptions without reraising.

@tunnell
Copy link
Contributor Author

tunnell commented Jan 28, 2021

This is misleading to new people if .xenon_secrets can't be found for some reason. They check if utilix installed, it is, but error is about utilix not in namespace.

@JoranAngevaare
Copy link
Contributor

JoranAngevaare commented Jan 29, 2021

Hi Chris, I tend to agree, I've also had incomprehensible errors due to this sloppy importing. I think the root issue is how utilix is initialized, rather than doing things sloppy here, I'd argue that we have to change this on the utilix side first.

By the way, there is a warning raised:

warn('Warning, utilix config file not loaded properly. copy '

I think these two PRs should work:

This was referenced Jan 29, 2021
@JoranAngevaare
Copy link
Contributor

JoranAngevaare commented Jan 29, 2021

I actually think the root issue is that we don't allow travis / github actions access to our database.

@tunnell I think we could fix all these issues and improve our testing if we allow access to our database using a read only account, I haven't found any convincing reason not to allow this access. True, we could setup some database in travis I don't have the time to do it while I can simply add an encrypted password and user into the testing (I know how to for both Travis and github actions).

This is becoming an increasingly pressing issue, for example, we cannot test the posrec algos because we set their files in our database.

If you approve (and possibly also @darrylmasson ), I will set this up (use a read-only that has access to the database to do tests). NB: by the way we are actually already a write account to do some uploads for admix.

@ershockley
Copy link
Contributor

@jorana we do have access to the database globally right? We can't write to the database but two of the mirrors are accessible. Or am I missing something?

@darrylmasson
Copy link
Contributor

Some accounts are limited in where they can be used from, but nt_analysis (for instance) has read-only access and can be used from anywhere.

@JoranAngevaare
Copy link
Contributor

JoranAngevaare commented Jan 29, 2021

Yeah, for some reason we have not used any database integration (because of performance/privacy/fear of malicious access??) . The problem is not that we cannot, the problem is if I'm not sure if the main database experts (@darrylmasson @tunnell ) are ok with the travis testing to have access to our database to read.

Apart from the performance I don't see any argument that makes sense. But even the performance should be fine.

@tunnell
Copy link
Contributor Author

tunnell commented Feb 3, 2021

Yes, that's fine. Just use TravisCI encrypted environmental variables. Write users have IP restrictions, so you'd have to use read-only.

That specific issue I raised should alter the try-import loop. Either check explicitely if TravisCI, which sets an environmental variables. If Travis, print warning. If not travis, raise exception. OR do what you're saying. I meant for a simpler solution. Catching two rather generic exceptions is just dangerous since the program can get in weird states. I didn't mean for big solution

@tunnell
Copy link
Contributor Author

tunnell commented Feb 3, 2021

That specific issue I raised should alter the try-import loop. Either check explicitely if TravisCI, which sets an environmental variables. If Travis, print warning. If not travis, raise exception. OR do what you're saying. I meant for a simpler solution

@JoranAngevaare JoranAngevaare added the bug Something isn't working label Feb 6, 2021
@JoranAngevaare
Copy link
Contributor

Solved in #360

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants