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

Fix caching for repos without remotes #194

Merged

Conversation

lakesare
Copy link
Contributor

SciUnit 0.2.4 (and up) wasn't working for me locally with the Backend (because I do have a .git repo, but don't have any remotes), this fixes it.

But I wonder - why do we need this, how is this useful for caching?

Put the commit # and origin of the current working copy into every model/test/capability created from a python session inside that working copy.
(#53).

image

@coveralls
Copy link

coveralls commented Jun 27, 2021

Coverage Status

Coverage increased (+0.09%) to 86.846% when pulling fdfd263 on lakesare:fix-caching-for-repos-without-remotes into 5a9312d on scidash:master.

@rgerkin
Copy link
Contributor

rgerkin commented Jun 28, 2021

@lakesare Happy to fix this, but I can't figure out the scenario you're encountering. If I run:

import sciunit
model = sciunit.Model()
model.get_remote()

it works whether I run this in an empty directory, a directory I've just run git init in (making it a git repo without a remote), or a directory whose .git/config has remotes. Can you produce a small recipe to reproduce the stack trace you showed above?

As for how it is useful for caching: in some applications (like SciDash) the source of the model(s) used determines the uniqueness of a test score. The serializer does not otherwise store the actual code of the model, so serializing the commit hash and repo is a proxy for the actual code. Any model with the same commit hash and repo, and the same attributes (from the rest of the serialization) is taken to be the same model.

@lakesare
Copy link
Contributor Author

lakesare commented Jun 29, 2021

You seem to have covered the scenario I encountered!
SciUnit v0.2.4 (and up), the code you used, and git init.
Might it be that you're using v0.23 locally?

In master we end up accessing repo.remotes[0], which python reacts aversely to.
I thought of creating a couple of tests, if you confirm you can't reproduce it with v0.24+ I'll create the tests.

Thank you for the explanation, makes perfect sense!
Maybe I'll add it as a comment to Versioned (unless you prefer more sparse commenting)?

@rgerkin
Copy link
Contributor

rgerkin commented Jun 29, 2021

@lakesare Now that you mention it I am actually using the current master branch of sciunit, which will be the future version 0.25. And it has some serialization code changes. Nothing I can think of that would change the git remote situation, though. You could try git cloning and installing it and see if that fixes things.

But I agree that repo.remotes[0] is a dangerous call. But wouldn't it have been sufficient to wrap the existing call in try/except block?

@lakesare
Copy link
Contributor Author

lakesare commented Jun 29, 2021

I'm running the current master too (this issue is present in both v0.2.4 and the current master).

But I agree that repo.remotes[0] is a dangerous call. But wouldn't it have been sufficient to wrap the existing call in try/except block?

It would be sufficient, but it would be worse too? It's not an exception semantically (not having any remotes is normal and expected), and exceptions are harder to debug.
Why, are you worried we might get merge conflicts?

@lakesare
Copy link
Contributor Author

I added some tests showing where the old version fails, see fdfd263#diff-4386d9251689717183d28bb7d126eec96d24d45406db360345df14b14eeccffcR88 (the tests pass on my machine).

If the tests fail on your machine then we might have different versions of gitpython.

@rgerkin
Copy link
Contributor

rgerkin commented Jun 30, 2021

@lakesare Can you remove old_get_remote and any test on this function? We don't need it anymore.

@rgerkin rgerkin merged commit 7e8635e into scidash:master Jul 1, 2021
@rgerkin
Copy link
Contributor

rgerkin commented Jul 1, 2021

Thank you, @lakesare!

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.

None yet

3 participants