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

Update docs to revise the dependencies page #2224

Closed
stichbury opened this issue Jan 19, 2023 · 6 comments · Fixed by #3772
Closed

Update docs to revise the dependencies page #2224

stichbury opened this issue Jan 19, 2023 · 6 comments · Fixed by #3772
Assignees
Labels
Component: Documentation 📄 Issue/PR for markdown and API documentation

Comments

@stichbury
Copy link
Contributor

@deepyaman pointed out that https://kedro.readthedocs.io/en/stable/kedro_project_setup/dependencies.html still documents the pip-compile workflow although it is not built in any more. So we need to revise this

@stichbury stichbury added the Component: Documentation 📄 Issue/PR for markdown and API documentation label Jan 19, 2023
@stichbury stichbury changed the title Update docs to edit the Update docs to edit the dependencies page and remove pip-compile workflow Jan 19, 2023
@astrojuanlu astrojuanlu added good first issue Good first issues for beginners and removed good first issue Good first issues for beginners labels Oct 6, 2023
@astrojuanlu
Copy link
Member

I don't quite understand - using pip-compile is the replacement of kedro build-reqs, right? It's not built-in but on the top of that document we're telling users to do pip install pip-tools, so it all looks fine to me.

@stichbury
Copy link
Contributor Author

Also to do, suggestion by @deepyaman

Why is https://docs.kedro.org/en/stable/kedro_project_setup/dependencies.html#workflow-dependencies section necessary? This is not documentation for contributors, it's for users, so they shouldn't need the dependencies for running the Kedro test suite or building the documentation. Alternatively, if they do (for some reason), we should explain it clearly.
This made a teensy bit of sense (not really) when it was a shortcut to install every dataset imaginable (at least that's user-facing functionality), but it seems like could just remove the section now.

@deepyaman
Copy link
Member

I don't quite understand - using pip-compile is the replacement of kedro build-reqs, right? It's not built-in but on the top of that document we're telling users to do pip install pip-tools, so it all looks fine to me.

I don't really remember the discussion (almost been a year), but I think it looks fine.

Tangentially, I'm not sure it's necessary to document the pip-compile here at all (when it was a functionality we promoted through kedro build-reqs, it makes sense to document it; when it's something extra you need to install, and represents just one way (and no the most popular one) of doing something that some people may consider best practice, it's probably unnecessary. If a new user were to read this question, unless they were already wondering, "How do I user lock files with Kedro?", one could argue the section is just confusing.

I'm fine with no action or removing the section; no strong feelings.

@stichbury
Copy link
Contributor Author

So there's no changes required? Or some changes to remove the section (maybe)?

I cannot make this decision as it's not my area of expertise, but it reads quite confusingly so if you think it is too, maybe it either needs rewriting or removal.

@deepyaman @astrojuanlu Please could you agree what is needed and let me know (I can execute the changes if that helps).

@astrojuanlu
Copy link
Member

astrojuanlu commented Oct 11, 2023

I think that page needs some love beyond removing the pip-compile workflow. I see the pip install "kedro[all]" is already being removed in #3156.

In general, if we want to encourage good practices, I think we should tell our users to dump the dependencies they want in requirements.txt (formerly src/requirements.txt before #2926) and tell them that they can do pip install -r requirements.txt. This is "common knowledge" anyway and would make it for a very short page if we only have that.

Then, the "Install dependencies related to the Data Catalog" part feels more of a kedro-datasets thing than a Kedro thing. We might want to keep those instructions for consistency, or move them somewhere else. But either way, we should not advise users to do pip install "kedro-datasets[<group>]", but instead add kedro-datasets[<group>] to requirements.txt and then have them do pip install -r requirements.txt again.

And finally, we could add an appendix on "reproducible environments", similar to https://docs.readthedocs.io/en/stable/guides/reproducible-builds.html#pin-your-transitive-dependencies, that advises users to use pip-compile. But that feels more of a "tips and tricks" than anything else.

In summary, I think just removing the pip-compile section from there is barely a net improvement and that we should rework that page when we have the time. This is probably too complex for a Hacktoberfest issue.

Does this make sense?

@stichbury stichbury changed the title Update docs to edit the dependencies page and remove pip-compile workflow Update docs to revise the dependencies page Oct 11, 2023
@stichbury
Copy link
Contributor Author

That alll sounds reasonable, thanks @astrojuanlu

I'll remove the label for now and come back to this when I can (page stats suggest this isn't a hugely visited page, but it definitely sounds like it needs some improving). Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Documentation 📄 Issue/PR for markdown and API documentation
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants