Skip to content

Conversation

@validbeck
Copy link
Collaborator

Internal Notes for Reviewers

For sc-5132, I updated the following:

All the links on these files should be updated to link out to the new restructured docs!

intro_for_model_developers_EXECUTED.ipynb

@nrichers FTR, make get-source overrode my edits to this notebook, so I had to retrieve them again (thankfully I sent up an atomic commit!). I don't know if the Makefile is checking against the main docs repo or the developer-framework repo (makes sense it would be the former?) so hopefully after this PR is merged that shouldn't happen again.

JupyterHub & Colab

As mentioned in PR#131 & PR#132 @cachafla will have to help with uploading the modified notebooks to JupyterHub.

I have updated the following file for Colab however:

  • quickstart_customer_churn_full_suite.ipynb

Notebooks not found:

Also, these are in the Colab folder but not actually in the notebooks zip anymore (probably got renamed?):

  • credit_risk_scorecard_demo.ipynb
  • Introduction_Customer_Churn.ipynb
  • nlp_sentiment_analysis_catboost_demo.ipynb
  • probability_of_default_validmind.ipynb
  • tutorial_time_series_forecasting.ipynb

Frontend

There's an open PR for this: https://github.com/validmind/frontend/pull/842

@validbeck validbeck added the internal Not to be externalized in the release notes label Jul 4, 2024
@validbeck validbeck self-assigned this Jul 4, 2024
@validbeck validbeck requested review from noosheenv and nrichers July 4, 2024 22:44
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.

Re: this bit in the main README.md: "tests — This is where tests retrieved from the developer-framework repo live."

I missed this in my original review, but we don't actually retrieve the tests. I can't comment on the line as it's not being changed in this PR, but a more accurate description would be:

This is where test descriptions generated from the Python source in the [developer-framework repo](https://github.com/validmind/developer-framework) live.

(We generate the test descriptions ourselves from the Python source via https://github.com/validmind/documentation/blob/main/site/Makefile#L68.)

Main ValidMind README

Minor: It looks like the updated QuickStart link there doesn't go to the QuickStart on JupyerHub directly. I assume that's intentional but, if not, it might make sense to have all the QuickStart links go to the same place for consistency.

@nrichers FTR, make get-source overrode my edits to this notebook, so I had to retrieve them again (thankfully I sent up an atomic commit!).

Yup, if you run make get-source, the notebook from our documentation main branch gets checked out. I'm sorry you ran into this issue — every exception or special case tends to bite us sooner or later and this one is, er, no exception. After we start including executed notebooks by default (or we sort out OAuth issues with JupyterHub), we can get rid of the special case.

As mentioned in validmind/validmind-library#131 & validmind/validmind-library#132 @cachafla will have to help with uploading the modified notebooks to JupyterHub

Either Spencer or Cullen can do this now, but we need to coordinate with Michael to make sure there are no demos going on, as redeploying will restart JupyterHub for everyone.

Also, these are in the Colab folder but not actually in the notebooks zip anymore (probably got renamed?)

Good catch! At this point, only the QuickStart should still be on Colab. I have removed the other notebooks and retested the QuickStart link.

@validbeck
Copy link
Collaborator Author

@nrichers

This is where test descriptions generated from the Python source in the developer-framework repo live.

Changed, thank you!

Minor: It looks like the updated QuickStart link there doesn't go to the QuickStart on JupyerHub directly. I assume that's intentional but, if not, it might make sense to have all the QuickStart links go to the same place for consistency.

Hm, it DOES go to the JupyterHub Quickstart — this was based on your link in another PR you suggested: https://docs.validmind.ai/get-started/developer/try-with-jupyterhub.html

Either Spencer or Cullen can do this now, but we need to coordinate with Michael to make sure there are no demos going on, as redeploying will restart JupyterHub for everyone.

Gotcha, I will make a follow-up story in Infra and tag everyone then 🫡

@validbeck validbeck merged commit 4467cc2 into main Jul 5, 2024
@validbeck validbeck deleted the beck/sc-5132/fix-broken-external-links-out-to-restructured branch July 5, 2024 15:33
@nrichers
Copy link
Collaborator

nrichers commented Jul 5, 2024

Hm, it DOES go to the JupyterHub Quickstart — this was based on your link in another PR you suggested: https://docs.validmind.ai/get-started/developer/try-with-jupyterhub.html

Are we looking at the same thing? I'm referring to the organization readme:

image

@validbeck
Copy link
Collaborator Author

Are we looking at the same thing? I'm referring to the organization readme:

Ah, I was looking at the documentation README. That was the original link in the top-level README; feel free to change it to the JH if you think that fits better!

@nrichers
Copy link
Collaborator

nrichers commented Jul 5, 2024

Ah, I was looking at the documentation README. That was the original link in the top-level README; feel free to change it to the JH if you think that fits better!

Done, via validmind/.github@f08fb60

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal Not to be externalized in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants