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 partial wrappers and deferred annotations #1895

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

Conversation

technillogue
Copy link
Contributor

@technillogue technillogue commented Aug 21, 2024

#1772 broke partial and partialmethod, as used in https://github.com/replicate/cog-llama-template/blob/main/predict.py#L234-L259. this attempts to fix that while maintaining support for deferred annotations

this PR was ready to be merged except for implementing the test for partialmethod schema generation being an unnecessarily slow integration test instead of a more lightweight test.

@technillogue technillogue force-pushed the syl/fix-deferred-annotations branch 8 times, most recently from c83a8e4 to 427149d Compare August 22, 2024 19:11
@technillogue technillogue requested a review from a team August 22, 2024 19:34
@@ -299,6 +299,20 @@ def test_predict_works_with_deferred_annotations():
timeout=DEFAULT_TIMEOUT,
)

def test_predict_works_with_partial_wrapper():
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be an integration test? Could it be a worker test instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just following #1772, should test_predict_works_with_deferred_annotations be moved as well?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right. I'd be tempted to move both of those into test_worker if it's possible to reproduce the original failure there. It's not really clear to me what the original failure was, though, so it's possible that doesn't work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://replicatehq.slack.com/archives/C05600FDYTE/p1723843667345019

    **get_input_create_model_kwargs(signature, input_types),
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/cog/predictor.py", line 306, in get_input_create_model_kwargs
    raise TypeError(f"No input type provided for parameter `{name}`.")
TypeError: No input type provided for parameter `prompt`.

and

  File "/root/.pyenv/versions/3.11.7/lib/python3.11/site-packages/cog/predictor.py", line 374, in get_input_type
    input_types = get_type_hints(predict)
                  ^^^^^^^^^^^^^^^^^^^^^^^
  File "/root/.pyenv/versions/3.11.7/lib/python3.11/typing.py", line 2381, in get_type_hints
    raise TypeError('{!r} is not a module, class, method, '
TypeError: functools.partial(<bound method Predictor.predict of <predict.Predictor object at 0x70abe38f0490>>) is not a module, class, method, or function.

Copy link
Contributor Author

@technillogue technillogue Aug 23, 2024

Choose a reason for hiding this comment

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

I think it needs to at least be a predictor test because I don't think worker calls get_input_type, though a schema test would work too

@technillogue
Copy link
Contributor Author

@nickstenning, if the delay from this test isn't prohibitive, I'd rather just merge this to unblock forks of the old cog-llama-template models, it might make sense to move several of these tests to server/test_predictor.py but that's a fair amount of new stuff

@nickstenning
Copy link
Member

nickstenning commented Aug 28, 2024

Given that you've still got to roll this thing out either way, I'm not sure merging a slow test is going to save much time. Let's not continue to add to the mess. Could we write some better unit tests for get_{input,output}_type instead?

We've leaned heavily into "can't we just" on the cog codebase for quite a while now, and it's got us into a right mess. I'd like us to start doing things we can support for the longer term.

@technillogue technillogue requested a review from a team November 4, 2024 20:02
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