Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
docs & readme: what/why/how #433
docs & readme: what/why/how #433
Changes from 15 commits
1bcd9fc
5368f7e
7cfe25b
6992d0e
76e11f6
f0b6814
e932990
a4e14d7
b1fa61b
2a11af5
66a5a42
794304c
c17f707
ddf4910
fb2ac4f
5b18120
40aabcc
87a76ec
97583de
6a0ffa2
68fe91d
ac1dfc4
3f81ab0
18f7130
c5c1f1b
d4b3736
3b05aa3
4381e65
474ace9
6b91aa3
9f2a105
aac92bc
fd848e2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking... what if a user does not have any ml training script on heirs hands? Or a user has a script but the environment settings will prevent user from executing it? Does it make sense to provide a script in additional to the TF file?
It should reduce the entrance bar quite significantly.
An ideal tutorial should have all the commands, scripts and data I need to run to get a result. Example - dvc tutorial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow, do you mean #305?
Do you mean the script won't execute locally or won't execute on the cloud? And why won't it execute? Because of missing env vars such as
NUM_EPOCHS
or something? Or missing dependencies likenumpy
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, user might not have any ml training script at all (or it won't work).
In DVC tutorial, user downloads code as the first step. Do we need something similar here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah so an example repo? No we don't have one (yet). Technically we'd probably need 3 - one each for AWS/Azure/GCP.
The current example simply uses AWS and an
echo Hello World
script, so the user just copy-pastes themain.tf
and has no other dependencies.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😬 Unless we use iterative/example-repos-dev or similar, it doesn't look like the best idea from a maintainability standpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that TPI is suppose abstract it out 😄 One code file with one "small" data file that user can get with
wget
should be enough.It would be great to have some "realistic" repo with checkpoint etc... like minst but slower :) Fashion-mnist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the only required change is
cloud = "whatever"
😅There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think at a minimum in example repos:
main.tf
:cloud = "X"
README.md
: "How to authenticate/export X_CREDENTIALS
, or use[cloud Y](link to example repo for Y)
or[cloud Z](link to example repo for Z)
"Plus probably:
requirements.txt
run.py
.github/workflows/cml.yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't think this complexity is needed in the readme here. Right now it's a self-contained1 example that supports users both with and without a script file.
Footnotes
apart from the how-to-setup-credentials external link ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broken, it seems
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, should be https://static.iterative.ai/img/cml/banner-tpi.svg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of https://static.iterative.ai/img/cml/banner-terraform.png
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See iterative/static#8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔔 @casperdcl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💅🏼 The title of this page is Task Resource but the nav entry is
iterative_task
. Seems inconsistentThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should probably be
Resource: iterative_task
like in other providers.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is Generic Machine Types under "Development" in the nav? (What does Development even refer to here?) Feels like machine types should be under the same section as the
iterative_task
res ref. somehow.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nav structure idea:
TPI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as abbreviations aren't part of the proposal, sounds good. The only thing I find rather dissonant is moving the Machine Types page under the Reference section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Abbreviations aren't part of the proposal; I was just being lazy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "Development" refer to as-is now? Maybe I'm not getting the current intended struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An XY solution would be disguising “Machine types“ as a guide (e.g. how to choose machine types) 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah either put it in the guide or in the ref IMO 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that the “Resources” section wat meant isn't a “Reference” in the general sense of the word; it's just a list of resources (hundreds in some providers).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can move is under “Resources”, but it's rather unorthodox. 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's fine, call the section Resources (just also use that word in links if possible, instead of "reference") And put machine types in the guide.