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

Add language model prices #501

Merged
merged 3 commits into from
Sep 13, 2024
Merged

Conversation

jlvallelonga
Copy link
Contributor

I'd love to refactor the code around this, but this is the quick way of accomplishing this. There might be more to it, but wanted to put this up for discussion first

@krschacht
Copy link
Contributor

I think this is a good enough solution. It's the right idea to start simple.

When we update a price of a model, we have to remember to do it in two places (the registration-create place and a migration to update all the old models). There is some slightly different architecture that could have us just update it in one place, but I can't immediately think of an easier one. I'm good just running with this.

If this PR is done feel free to merge in. It looked good to me.

@jlvallelonga jlvallelonga marked this pull request as ready for review September 12, 2024 00:53
@krschacht krschacht merged commit 1396760 into main Sep 13, 2024
5 of 6 checks passed
@krschacht krschacht deleted the feature/add-language-model-prices branch September 13, 2024 22:46
@krschacht
Copy link
Contributor

Just merged

@jlvallelonga
Copy link
Contributor Author

Thanks. I was struggling to get the system tests to pass.

@krschacht
Copy link
Contributor

Yes, the system tests are still an issue. Rob didn’t completely figure it out. However, he identified the root cause even though he didn’t know how to fix it. Actually this week there were a couple comments on this issue I opened with Capybara: teamcapybara/capybara#2770 Multiple people report the same issue with multiple fixes. The simplest one seems to be pinning to a previous version of the driver, which would explain why things started failing all the sudden. It was always my hunch that something in the CI system got upgraded.

Anyway, I mention this just in case you get around to looking into it before me. The billing thread is still the higher priority, but it’ll be nice when we finally get these system tests fixed so it’s an alternative if you want to switch it up, or sometime next week I’ll try these fixes.

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