Set the default repo_path inside the get_model_spec function

[jzuhone]

Description

This PR makes two changes one minor change to xija.get_model_spec. one minor and another that is API-breaking

1. Let the default for repo_path be None and then set it to REPO_PATH internally, so that calling routines from other packages can pass a None as default and they won't need to know about REPO_PATH.
   2. In a couple of cases, I have found that it would be nice to know the path to the JSON file returned by get_model_spec, so it can be reported in logging or print statements. The way I currently do this is rather clunky--I import REPO_PATH from the same module. But this duplicates what's already being done in the function and is error-prone if something changes. So this change simply returns the path as the third argument--unfortunately this also breaks API.

If anyone can think of a non-API-breaking way to achieve the second change, let me know.

[jzuhone]

Pinging @taldcroft @javierggt @jeanconn @matthewdahmer @jskrist (by the way, is it possible for me to be given permission to add reviewers on the right side? Currently I am not allowed)

[jskrist]

Would it make more sense to refactor the code a bit? i.e. extract out the bit of code that does the file globing into a separate public utility function that could be called explicitly, like get_full_spec_file_path(model_name, repo_path) and then this could be called internally and externally without breaking any APIs.

[jskrist]

might as well update the example in the doc string to show this new return value (or add a second example.

[javierggt]

Is this change a bugfix that should go into ska3-matlab 2021.7?

[jzuhone]

@javierggt I wouldn't call this a bugfix. it's not urgent. It depends on your timeline though.

[jzuhone]

I really like @jskrist's idea to factor the file path bit out so we don't have to wreck the API. I'll do that instead.

[taldcroft]

Also be aware that it always makes a clone into a temp dir. I didn't look at it very closely but maybe the file path you get will be useless.

[jzuhone]

Maybe we just need to log more info about the model used to the screen?

[taldcroft]

One more comment on the file name is that the basic philosophy behind all this is that the content is what matters and that it lives in a distributed git repo. So the version tag you get back from that repo (e.g. chandra_models version: 3.35.2) is effectively a signed guarantee of the contents.

It probably is not obvious, but you can do somewhat tricky things with this infrastructure. For instance, ACIS could add a brand new model to the repo in a dev branch named bep_fep. Then you could run load review for BEP_PCB from that branch which has files that don't even exist in the flight tag branch.

In [1]: from xija.get_model_spec import get_xija_model_spec                                                                                  

In [2]: spec, version = get_xija_model_spec('bep_pcb', version='bep_fep')                                                                    

In [3]: version                                                                                                                              
Out[3]: 'bep_fep'

Small note before you try this at home, namely that bep_fep branch right now is on the jzuhone remote, so to do this for real we would need to take sot/chandra_models#62 and put that into a sot/bep_fep branch.

[jzuhone]

@taldcroft I see the point now and I am reverting the changes in this PR which involve passing back the model path--I left in the change which sets the default repo_path inside the function so that calling functions can pass None. I'm thinking of the case in which the calling function has its own optional argument for repo_path which may be None or something else (planning this for acis_thermal_check).

[jskrist]

Is this a bug fix (correcting the annotated return type)? I originally though that you were changing the return type of the function, but now I don't see that in the diff. same for Line 95.

[jzuhone]

Yes, this a bug fix since it already returns a tuple.

[taldcroft]

Looks good thanks @jzuhone .