Skip to content

Revised before you begin page #108

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

Closed
wants to merge 6 commits into from
Closed

Revised before you begin page #108

wants to merge 6 commits into from

Conversation

p-m-s-f
Copy link
Contributor

@p-m-s-f p-m-s-f commented Jun 28, 2023

Responding to this story.

I've moved onto the second draft. At Nik's request, I've greatly shortened the length of the page, and removed some references to Google Colab. I've also listened to Anil's suggestion to link to the page describing how to create documentation projects.

image

Also, I spotted (and fixed) a typo on our customer churn quickstart page.

image

@p-m-s-f p-m-s-f requested a review from nrichers June 28, 2023 01:37
@p-m-s-f
Copy link
Contributor Author

p-m-s-f commented Jun 28, 2023

revised my first draft of the page, moving onto embedding cells from the before you begin page to other pages regarding jupyter notebooks

@nrichers
Copy link
Collaborator

Great timing! I'd started to review your initial commit but then got sidetracked. I'll take a look at this updated version now, @p-m-s-f.

Copy link
Collaborator

@nrichers nrichers 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 pushing an updated PR, @p-m-s-f! This looks much cleaner than the first version, but it now actually looks too sparse to actually work. I don't think this notebook actually could run, can it? I think we will need at least the code cells you removed back in the notebook, as they include important code for the notebooks to be executable. After that, we can figure out how to simplify the English instructions. I do laud your effort to clean things up, though!

Here's a screenshot in this comment to show what we might need back in. At a minimum, the code cells need to be included.

image

These instructions are from https://github.com/validmind/demo-environment/blob/main/demo_files/Quickstart_Customer%20Churn_full_suite.ipynb.

As for the English instructions, I don't think we want the current ones back in verbatim, as that is quite a bit of text. So the magic trick will be to figure out how little instructions we can get away with (and link to the docs for the rest, as you have done).

As an aside, you might need to merge main into your PR branch or rebase your commits on top of main (with some assistance from me), as you appear to be missing some stuff. E.g. your PR looks to remove the PR template Sydney added last week and the CI checks — we should not be deleting these. I'm not 100% sure what happened, but happy to take a look at it with you tomorrow.

@p-m-s-f
Copy link
Contributor Author

p-m-s-f commented Jun 29, 2023

i've updated the page with the requested code snippets. i'll spend more time tomorrow going over the written instructions

image

Copy link
Collaborator

@nrichers nrichers left a comment

Choose a reason for hiding this comment

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

Getting closer! I left a comment about adding a tip to create a documentation project (you need one to initialize the framework). I think that is the only bit that we definitely still need.

As part of your PR, can you add and test out your steps to make sure they work? E.g. you could update site/notebooks/nlp/nlp_sentiment_analysis_demo.ipynb with your before-you-begin snippet and then run all the cells.

p-m-s-f and others added 2 commits June 30, 2023 12:14
@p-m-s-f
Copy link
Contributor Author

p-m-s-f commented Jun 30, 2023

i created a documentation project but i wasn't able to get the code working-- perhaps this is something to return to on tuesday? i'm not the most familiar with the developer framework. i've included a copy of the customer churn notebook with my before you begin section, it should have the error messages intact.

image

Copy link
Collaborator

@nrichers nrichers left a comment

Choose a reason for hiding this comment

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

Nice updates for this pull request, @p-m-s-f!

Just parking some notes here for discussion with you:

  • Minor text edits (product names are proper nouns, add some info about why creating a documentation project matters, don't send people to Support; have just one tip)

  • Figure out why this PR seems to delete some stuff we definitely need (e.g. the pull request template)

We can chat on Zoom a bit today, if you like, but generally this PR is very close to where it needs to be before we roll out the changes.

@nrichers
Copy link
Collaborator

nrichers commented Jul 4, 2023

Replaced by #111.

@nrichers nrichers closed this Jul 4, 2023
sydneysugar pushed a commit that referenced this pull request Jul 11, 2023
@nrichers nrichers deleted the before_you_begin branch October 6, 2023 16:20
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