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: ignore "train" part of schema errors during predict runs #1496

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

dkhokhlov
Copy link
Contributor

@dkhokhlov dkhokhlov commented Jan 24, 2024

  • ignore "train" part of schema errors during predict runs for backward compatibility with existing "bad" models already in use
  • expected: cog shall not trigger exception/error when "train" is present in config but "train.py" is missing.

@dkhokhlov dkhokhlov force-pushed the ignore_schema_exceptions_on_trainer_path branch 2 times, most recently from 8daf55d to 6a65211 Compare January 24, 2024 20:55
@dkhokhlov dkhokhlov changed the title fix: ignore "train" part of schema errors during predict runs in cloud fix: ignore "train" part of schema errors during predict runs Jan 24, 2024
Copy link
Contributor

@mattt mattt left a comment

Choose a reason for hiding this comment

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

Nice work, @dkhokhlov. Thanks for taking this on.

Left a few non-blocking suggestions for renaming predictor_ref to training_ref. But other than that, I think this is solid.

Comment on lines 107 to +108
mode: str = "predict",
is_build: bool = False
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the decision to define this behavior in a new argument to create_app. We originally discussed defining a new mode value, but I see the value of using something that doesn't map to a command-line argument, and therefore risk misuse (e.g. What's the expected behavior of a pod running a model with the build mode?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not wild about this particular spelling, but I can't think of anything better. Since it's effectively a private API, it's probably fine. Or else, we can change it later without breaking anyone.

python/cog/server/http.py Outdated Show resolved Hide resolved
python/cog/server/http.py Outdated Show resolved Hide resolved
…uction for backward compatibility

Signed-off-by: Dmitri Khokhlov <dkhokhlov@gmail.com>
@dkhokhlov dkhokhlov force-pushed the ignore_schema_exceptions_on_trainer_path branch from 6a65211 to d60370e Compare January 24, 2024 21:44
@dkhokhlov dkhokhlov merged commit d255445 into main Jan 24, 2024
14 checks passed
@dkhokhlov dkhokhlov deleted the ignore_schema_exceptions_on_trainer_path branch January 24, 2024 21:58
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