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

Create CLI interface for tf-cloud #283

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

eddydecena
Copy link

  • Create CLI interface based on typer in top of click (docs: https://typer.tiangolo.com/)
  • Create a command to get remote (something like: tfc remote)
  • Create a command to run (something like: tfc run --entry-point mnist_example.py)

- Create CLI interface based on typer in top of click (docs: https://typer.tiangolo.com/)
- Create command for get remote (something like: tfc remote)
- Create command for run (something like: tfc run --entry-point mnist_example.py)
@google-cla google-cla bot added the cla: yes label Jan 20, 2021
@eddydecena
Copy link
Author

This pull request is about issue #279.

Copy link
Collaborator

@SinaChavoshi SinaChavoshi left a comment

Choose a reason for hiding this comment

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

Hi Eddy,

Thank you for the contribution. I have three specific requests:

  1. Could you move the cli.py module under cloud/src/python/tensorflow_cloud/experimental/cli - I would like to keep it under experimental until we see user feedback and utilization.

  2. Please use soft dependency instead of additions to dependancy.py. Specifically you can check for installed modules or attempt to import the required module for the cli.py and raise an error with corresponding instructions for the user when user uses the cli.py module and the required dependancies do not exist.

  3. [Optional - though highly recommended] Do you mind adding a short readme with instructions on how to generate / update the cli.py module for future developers / maintainers.

1. Could you move the cli.py module under cloud/src/python/tensorflow_cloud/experimental/cli - I would like to keep it under experimental until we see user feedback and utilization.

2. Please use soft dependency instead of additions to dependancy.py. Specifically you can check for installed modules or attempt to import the required module for the cli.py and raise an error with corresponding instructions for the user when user uses the cli.py module and the required dependancies do not exist.

3. [Optional - though highly recommended] Do you mind adding a short readme with instructions on how to generate / update the cli.py module for future developers / maintainers.
1. Fix link error for instalations
2. Fix link error for Typer documentation
Create dependency for typer as soft dependency
@eddydecena
Copy link
Author

@SinaChavoshi Let me know any other requirements

Copy link
Collaborator

@SinaChavoshi SinaChavoshi left a comment

Choose a reason for hiding this comment

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

Thank you for the update :)

I have one follow up, seems like there are two copies of cli.py, the original is still remaining in src/python/tensorflow_cloud/core/cli.py could you remove that and only use src/python/tensorflow_cloud/experimental/cli/cli.py.

1. cli.py file was move to src/python/tensorflow_cloud/experimental/cli.py
@eddydecena
Copy link
Author

@SinaChavoshi Done

Copy link
Collaborator

@SinaChavoshi SinaChavoshi left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! great work :)

@SinaChavoshi SinaChavoshi added the ready to pull PR ready for merge process label Apr 14, 2021
@eddydecena
Copy link
Author

Thank you for approving this feature, let's keep improving it.

@eddydecena
Copy link
Author

@SinaChavoshi What's going on? How can I fix that error from import/copybara?

@SinaChavoshi SinaChavoshi added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready to pull PR ready for merge process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants