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 support to pass model definition as a str #66

Closed
wants to merge 2 commits into from

Conversation

mark-thm
Copy link

@mark-thm mark-thm commented Jan 2, 2024

Presently, lleaves only supports passing model definitions as files, but in our use-case we store the model definitions in S3, which requires us to copy the definition to the local machine before loading and compiling. Rather than require this additional copy, this PR updates the code to support either passing a model_file, or passing the string content of that file directly as model_str.

If there's concern about the size of the string in memory, or there's a desire to accept broader stream formats I'd be happy to update the PR to take a StringIO at the top level instead of a str.

@siboehm siboehm self-requested a review January 3, 2024 00:49
Copy link
Owner

@siboehm siboehm left a comment

Choose a reason for hiding this comment

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

lgtm. Can you add a test? Just to make sure no one accidentally breaks the feature in the future

"""
self.model_file = model_file
if model_file is not None and model_str is not None:
Copy link
Owner

Choose a reason for hiding this comment

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

maybe merge this with the below if statements

@siboehm
Copy link
Owner

siboehm commented Jan 3, 2024

Thx Mark! I've gotten this request for string inputs now from multiple people, so maybe it's time to add. Thanks for implementing it. Previously I've always told be people to just use Temp files in a context manager. Would that solution work for you?

I'm not too worried about mem demand, the compiler itself already uses enormous amounts of mem and model.txt's are a few mb at most.

@mark-thm
Copy link
Author

mark-thm commented Jan 3, 2024

I'm currently just writing to a temp file in a context manager -- if you'd like to keep the interface simple I can stick with that. Figured this was a small-ish change so I'd throw it up but no big deal either way.

@siboehm
Copy link
Owner

siboehm commented Jan 3, 2024

Oh ok, if it's not a dealbreaker then I'd rather keep the interface small and add a note to the docs instead. Just keeps the codebase smaller and makes it easier for me :)

@mark-thm mark-thm closed this Jan 3, 2024
@mark-thm mark-thm deleted the me/model-str branch January 3, 2024 02:26
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