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

NLP use-case template without Model Control Plane #1

Merged
merged 19 commits into from
Nov 3, 2023

Conversation

safoinme
Copy link
Collaborator

No description provided.

@safoinme safoinme requested review from strickvl and fa9r October 28, 2023 16:16
Copy link

@htahir1 htahir1 left a comment

Choose a reason for hiding this comment

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

i would say we merge this and make a tag asap :-)

Copy link

@htahir1 htahir1 left a comment

Choose a reason for hiding this comment

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

The README could do with a bit of love to go into more detail

@strickvl strickvl added the enhancement New feature or request label Nov 1, 2023
@strickvl strickvl changed the title NLP use-case template without Model Control Plan NLP use-case template without Model Control Plane Nov 1, 2023
Copy link
Contributor

@strickvl strickvl left a comment

Choose a reason for hiding this comment

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

Looks really nice. I think once you take a look at these I'd want to test it out as well but basically looks good aside from these small nits.

copier.yml Outdated Show resolved Hide resolved
copier.yml Outdated Show resolved Hide resolved
zenml stack set $${stack_name} && \
zenml stack up

install-remote-stack:
Copy link
Contributor

Choose a reason for hiding this comment

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

left in by accident?

template/gradio/app.py Show resolved Hide resolved
template/pipelines/deploying.py Outdated Show resolved Hide resolved
template/utils/misc.py Show resolved Hide resolved
| Notifications on failure | Whether to notify about pipeline failures | True |
| Notifications on success | Whether to notify about pipeline successes | False |
| ZenML Server URL | Optional URL of a remote ZenML server for support scripts | - |
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we use this anywhere?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
## Next Steps

Once you have generated the project using this template, you can explore the generated code and customize it to fit your specific NLP use case. The README.md file in the generated project provides further instructions on how to set up and run the project.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just realised that there's also this (template/README.md) that is already merged on the repo etc. I am not sure if it needs revising or not. Seems like maybe it does? In any case, probably needs attention.

@strickvl
Copy link
Contributor

strickvl commented Nov 1, 2023

Also since this is a template that'll be used in lots of different ways, might be worth making the extra effort to add docstrings etc in (+ maybe helped by darglint to make sure you've actually done it etc).

Copy link

@htahir1 htahir1 left a comment

Choose a reason for hiding this comment

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

Also wanted to add that a lot of times youre using list or dict instead of typing.List etc (dosnt work on p38) and some imports i found missing sometimes (like Annotated)... Might want to recheck all that too

@safoinme safoinme merged commit 8ed95a7 into main Nov 3, 2023
1 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants