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

Metric versioning #93

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jordiclive
Copy link

A possible way to solve Issue #59.

We could make sure metric_version.py is run every new commit and then each metric can have an attribute loaded from the resulting json or yaml file.

metric_version.py finds the last commit that modified the respective metric file.

@tuetschek
Copy link
Collaborator

@jordiclive thanks, this looks like a good idea! I'd have a few questions & proposals:

  • Could we perhaps keep this right under gem_metrics/? The impl/ subdirectory only has some additional code for some of the metrics.
  • It would be good if the location of the json file was given by os.path.join(os.path.abspath(os.path.dirname('__file__')), 'metric_version.json') (or something more elegant along the same lines), so no matter where you call the script from, it always saves into the same path.
  • Maybe we also want to have a function that returns the version of a metric? Or maybe even a method in AbstractMetric?
  • Maybe we should put a list of all available metrics in one place somewhere into gem_metrics/__init__.py? It's actually kinda weird that we don't have it yet, and it might be useful. There's the metric_name_to_metric_class dict, so maybe base it on that?
  • Do you think there's a way of automating this somehow?

@jordiclive
Copy link
Author

jordiclive commented Apr 19, 2022

@tuetschek. Thanks for taking a look :) Yep, definitely agree with all those!

With automating, if you meanmetric_version.py to run every new commit, I imagine a git hook?

@tuetschek
Copy link
Collaborator

@jordiclive Yeah that's what I meant – but I'm actually not sure how to do it, since you first need to commit the metric and then run the script, which would essentially need another commit to store the version hashes 🤔?

@jordiclive
Copy link
Author

@tuetschek, yes you are right. And would be getting quite complicated! This is probably something for setuptools or having json saved somewhere else.

@jordiclive jordiclive closed this Apr 22, 2022
@tuetschek
Copy link
Collaborator

@jordiclive Does this mean you think it's too complicated now 🙂? I guess we could just have some tests that would essentially force you to commit the JSON every time you're changing a metric file... but yeah, maybe it would take time to figure out.

@jordiclive
Copy link
Author

jordiclive commented Apr 26, 2022

Ha, no I'm happy to continue! I just thought there may be a more elegant way with setuptools.

Maybe if a commit is logged before the force commit. As we need two commits and then it to stop. I think it is possible with a post-commit hook. I will look into that.

@jordiclive jordiclive reopened this Apr 26, 2022
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