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

NMMA models - multiple sources #279

Merged
merged 7 commits into from
Nov 29, 2023

Conversation

Theodlz
Copy link
Collaborator

@Theodlz Theodlz commented Nov 29, 2023

This PR makes it possible to point either to gitlab or zenodo to fetch the models.

We make gitlab the default, as this is the one we are able to actively update. Also, we make sure that a failure to use one source of models triggers a retry with another source, until we run out of options. Here we have just 2 of them, but we can imagine having multiple ones in the future.

Maybe, later on, we'll want to have the models.yaml file live in a seperate repo or NMMA itself, and have a pointer to each source for each models, to have a more robust system. Something to think about.

For context, we opted for gitlab to bypass the very strict upload and bandwidth limits imposed by github when using git lfs (a module that allows for the upload of large binary files). Here's the gitlab repo: https://gitlab.com/Theodlz/nmma-models. There, we make use of compression to reduce the individual file size. This should help with downloading speeds, as well as maintaining the repo's full size small enough.

Also added some tests to improve the coverage a little bit :)

Copy link
Collaborator

@bfhealy bfhealy left a comment

Choose a reason for hiding this comment

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

LGTM!

@Theodlz
Copy link
Collaborator Author

Theodlz commented Nov 29, 2023

LGTM!

Thanks for the review Brian! Just to be sure, did that new code work fine for you (i.e. have you had a chance to try the code0?

@bfhealy
Copy link
Collaborator

bfhealy commented Nov 29, 2023

@Theodlz Yes, I tried it out by forcing the source to be zenodo and gitlab and it worked in both cases. It did help me identify an unrelated issue with analysis.py that I'll be posting about soon.

@Theodlz
Copy link
Collaborator Author

Theodlz commented Nov 29, 2023

@Theodlz Yes, I tried it out by forcing the source to be zenodo and gitlab and it worked in both cases. It did help me identify an unrelated issue with analysis.py that I'll be posting about soon.

Great! Do you need that issue to be addressed before this is merged, or should I wait before merging this?

@bfhealy
Copy link
Collaborator

bfhealy commented Nov 29, 2023

I run into this issue (#280) even on main, so I think you're good to merge this even before we resolve it.

Edit - wrong issue number

@Theodlz
Copy link
Collaborator Author

Theodlz commented Nov 29, 2023

I run into this issue (#280) even on main, so I think you're good to merge this even before we resolve it.

Edit - wrong issue number

Noted! Thanks a lot for the review, and let me know if you want me to look at the code of the PR you might open.

@Theodlz Theodlz merged commit 0e5e43e into nuclear-multimessenger-astronomy:main Nov 29, 2023
5 checks passed
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.

2 participants