feat: add price estimation #400

newokaerinasai · 2025-11-25T13:13:45Z

Have you read the Contributing Guidelines?

Issue #

Describe your changes

This adds the price estimation functionality to the client. This also adds a warning if the estimated price is too high.

nikita-smetanin · 2025-11-25T15:53:11Z

src/together/cli/api/finetune.py

+    "greater than your current credit limit and balance. "
+    "It will likely fail due to insufficient funds. "
+    "Please consider increasing your credit limit at https://api.together.xyz/settings/profile\n"
+    "You can pass `-y` or `--confirm` to your command to skip this message.\n\n"


Should we also warn that a job will fail anyway? And not like execute with negative balance

Wdym? There's "It will likely fail due to insufficient funds" is above?

src/together/cli/api/finetune.py

src/together/resources/finetune.py

nikita-smetanin · 2025-11-25T15:57:24Z

src/together/resources/finetune.py

+        Returns:
+            FinetunePriceEstimationResponse: Object containing the estimated price.
+        """
+        training_type_cls: TrainingType | None = None


We don't need to set them None here, as they're defined below in all branches (or exception). You can keep type definition if you want to

mypy complains

That's a bit weird, all the possible branches are covered below.

Does it complain when you don't define the type or when you don't set it to None?

nikita-smetanin · 2025-11-25T15:58:19Z

src/together/types/finetune.py

+    model: str
+    n_epochs: int | None = None
+    n_evals: int | None = None
+    training_type: TrainingType | None = None


Can it be none? Same goes for n_epoch and n_evals?

It can be None. If I implement it in any other way, it would be an unrelated change -- I'd just need to change it for all other data types for FT

nikita-smetanin · 2025-11-25T15:58:47Z

src/together/resources/finetune.py

+        self,
+        *,
+        training_file: str,
+        model: str | None,


Can these fields be none?

They can in principle, but the price estimation API doesn't seem to support it. Therefore, we need to fix the API, disable calling the estimation when model is None, but set this field to always be defined (i.e., remove the None option)

src/together/resources/finetune.py

src/together/cli/api/finetune.py

mryab

It would be nice to supplement the client changes with some simple unit tests!

mryab · 2025-11-26T15:02:00Z

src/together/resources/finetune.py

+    "It will likely fail due to insufficient funds. "
+    "Please proceed at your own risk."


Suggested change

"It will likely fail due to insufficient funds. "

"Please proceed at your own risk."

"It will likely get cancelled due to insufficient funds. "

"Proceed at your own risk."

mryab · 2025-11-26T15:09:05Z

src/together/resources/finetune.py

+        Returns:
+            FinetunePriceEstimationResponse: Object containing the estimated price.
+        """
+        training_type_cls: TrainingType | None = None


Does it complain when you don't define the type or when you don't set it to None?

src/together/resources/finetune.py

artek0chumak · 2025-12-01T10:57:22Z

src/together/resources/finetune.py

            hf_api_token=hf_api_token,
            hf_output_repo_name=hf_output_repo_name,
        )
+        if from_checkpoint is None:


Suggested change

if from_checkpoint is None:

if from_checkpoint is None and from_hf_model is None:

I don't think you support from_hf_model either (estimated price will be incorect due to wrong parameter count)

nikita-smetanin

Nice!

src/together/cli/api/finetune.py

mryab · 2025-12-03T23:34:37Z

src/together/resources/finetune.py

+        Estimates the price of a fine-tuning job
+
+        Args:
+            request (FinetunePriceEstimationRequest): Request object containing the parameters for the price estimation.


The docstring is not consistent with the actual arguments

mryab · 2025-12-03T23:34:48Z

src/together/resources/finetune.py

+        Estimates the price of a fine-tuning job
+
+        Args:
+            request (FinetunePriceEstimationRequest): Request object containing the parameters for the price estimation.


mryab · 2025-12-03T23:37:53Z

src/together/resources/finetune.py

+            # unsupported case
+            price_estimation_result = FinetunePriceEstimationResponse(
+                estimated_total_price=0.0,
+                allowed_to_proceed=True,
+                estimated_train_token_count=0,
+                estimated_eval_token_count=0,
+                user_limit=0.0,
+            )


Nit: I would create a separate variable price_limit_passed and assign price_limit_passed = price_estimation_result.allowed_to_proceed when there is no CFT but just price_limit_passed = True in the unsupported case. This would prevent creating a dummy object with arbitrary values across the file

mryab · 2025-12-03T23:40:41Z

tests/unit/test_finetune_resources.py

+    )
+
+    assert mock_requestor.request.call_count == 3
+    assert response.id == "ft-12345678-1234-1234-1234-1234567890ab"


Let's extract this into a constant perhaps? It's used 3 times across the file, might easily diverge or not get used in future tests

mryab · 2025-12-03T23:42:25Z

tests/unit/test_finetune_resources.py

+    else:
+        return (


Better to explicitly check that it's equal to /limits (since this is what we return here) and raise ValueError if none of the options would match. A dispatch table pattern is a good option here

mryab · 2025-12-03T23:43:45Z

tests/unit/test_finetune_resources.py

+    mocker.return_value = (
+        TogetherResponse(
+            data={
+                "estimated_total_price": 100,
+                "allowed_to_proceed": True,
+                "estimated_train_token_count": 1000,
+                "estimated_eval_token_count": 100,
+                "user_limit": 1000,
+            },
+            headers={},
+        ),
+        None,
+        None,
+    )


Hm, it feels weird that we can't reuse mock_request here and have to define the mocking logic once again. I wonder if we can generalize somehow…

…computer/together-python into ruslan/price-estimation

Co-authored-by: Max Ryabinin <mryabinin0@gmail.com>

add price estimation

efefbbb

newokaerinasai requested review from artek0chumak and nikita-smetanin November 25, 2025 13:22

newokaerinasai marked this pull request as ready for review November 25, 2025 13:22

nikita-smetanin requested changes Nov 25, 2025

View reviewed changes

comments from the review

2ee9bbc

artek0chumak reviewed Nov 26, 2025

View reviewed changes

src/together/cli/api/finetune.py Outdated Show resolved Hide resolved

mryab requested changes Nov 26, 2025

View reviewed changes

src/together/cli/api/finetune.py Outdated Show resolved Hide resolved

newokaerinasai added 3 commits November 26, 2025 14:04

адк

d35e770

code style

5eaea00

add combined

f73533d

mryab reviewed Nov 26, 2025

View reviewed changes

newokaerinasai added 3 commits November 26, 2025 17:29

address None and comments from review

dea7fdf

Update test_finetune_resources.py

a422b53

Update test_finetune_resources.py

ca42382

artek0chumak self-requested a review December 1, 2025 10:55

artek0chumak reviewed Dec 1, 2025

View reviewed changes

artek0chumak approved these changes Dec 1, 2025

View reviewed changes

fix from_checkpoint and hf_model

3c11c5f

nikita-smetanin approved these changes Dec 3, 2025

View reviewed changes

Merge branch 'main' into ruslan/price-estimation

faffc82

mryab reviewed Dec 3, 2025

View reviewed changes

newokaerinasai and others added 3 commits December 3, 2025 16:05

comments from code review

dc923a4

Merge branch 'ruslan/price-estimation' of https://github.com/together…

9897bc6

…computer/together-python into ruslan/price-estimation

Apply suggestions from code review

aa0d8e7

Co-authored-by: Max Ryabinin <mryabinin0@gmail.com>

mryab approved these changes Dec 4, 2025

View reviewed changes

newokaerinasai merged commit e465a0e into main Dec 4, 2025
11 checks passed

newokaerinasai deleted the ruslan/price-estimation branch December 4, 2025 02:04

		"It will likely fail due to insufficient funds. "
		"Please proceed at your own risk."

	if from_checkpoint is None:
	if from_checkpoint is None and from_hf_model is None:

feat: add price estimation #400

feat: add price estimation #400

Uh oh!

Conversation

newokaerinasai commented Nov 25, 2025

Describe your changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mryab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikita-smetanin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants