Skip to content
This repository has been archived by the owner on Sep 24, 2024. It is now read-only.

Added v0 of prometheus lm-buddy entrypoint #75

Merged
merged 19 commits into from
Mar 12, 2024
Merged

Conversation

aittalam
Copy link
Member

Here's the first version of the lm-buddy prometheus entrypoint!

I tried to keep the script relatively close to the original evaluation script, as I'd like to use it as a use case for a "conversion" of some existing tool (a tutorial is being written...)

I feel it still needs some massaging as I am not 100% sure of the implementation choices I took (format of configs, using one method or another to get artifacts, etc). Happy to receive some early feedback and iterate on it soon!

Copy link
Contributor

@sfriedowitz sfriedowitz left a comment

Choose a reason for hiding this comment

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

Looking good overall with the format of the new job! A handful of comments, some bigger than others. Happy to discuss more or chat offline if anything didn't make sense.

src/lm_buddy/jobs/configs/prometheus.py Outdated Show resolved Hide resolved
src/lm_buddy/jobs/_entrypoints/prometheus.py Outdated Show resolved Hide resolved
src/lm_buddy/jobs/_entrypoints/prometheus.py Outdated Show resolved Hide resolved
src/lm_buddy/jobs/_entrypoints/prometheus.py Outdated Show resolved Hide resolved
src/lm_buddy/jobs/configs/prometheus.py Outdated Show resolved Hide resolved
…ean's comment

Co-authored-by: Sean Friedowitz <[email protected]>
Signed-off-by: Davide Eynard <[email protected]>
@aittalam
Copy link
Member Author

aittalam commented Mar 1, 2024

Many thanks for your comments! I will go through them (marked all of the actionable ones with a 👍) and also fix the linting issues before asking for approval 🙏

Copy link
Contributor

@sfriedowitz sfriedowitz left a comment

Choose a reason for hiding this comment

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

I'll approve this for now as most comments have been addressed. I left a few other smaller nit comments you may want to tackle as well in this PR.

In a followup, I think it would be good to take another iteration at the following:

  • Refining how we store generated results to disk, and the format of the W&B artifacts. E.g., do we log cumulative metrics from the eval as a Run Table for easy visualization, in addition to the file artifact?
  • Adding test coverage, both for ser/de issues on the new configs, and for the job entrypoint as a whole.

Copy link
Member

@veekaybee veekaybee left a comment

Choose a reason for hiding this comment

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

🚀

@aittalam aittalam merged commit 88e606a into main Mar 12, 2024
4 checks passed
@aittalam aittalam deleted the davide/prometheus branch March 12, 2024 21:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants