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.

Sign up for GitHub

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

Jump to bottom

Update documentations of Configuration default environments #3321

Merged
merged 21 commits into from
Dec 7, 2023
Merged

Update documentations of Configuration default environments #3321

merged 21 commits into from
Dec 7, 2023

Conversation

noklam
Copy link
Contributor

@noklam noklam commented Nov 20, 2023 •
edited
Loading

NOTE: Kedro datasets are moving from kedro.extras.datasets to a separate kedro-datasets package in
kedro-plugins repository. Any changes to the dataset implementations
should be done by opening a pull request in that repository.

Description

Development notes

Part of #3311

Developer Certificate of Origin

We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a Signed-off-by line in the commit message. See our wiki for guidance.

If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.

Checklist

  • Read the contributing guidelines
  • Signed off each commit with a Developer Certificate of Origin (DCO)
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes
  • Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

@noklam
update docs
a9c9c36 
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam noklam mentioned this pull request Nov 20, 2023
Merged
12 tasks
@noklam noklam changed the base branch from main to develop November 21, 2023 04:19
@noklam noklam self-assigned this Nov 22, 2023
@noklam
add docs
c902418 
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam
bubble up the common features and fix some level of headers
e6b9587 
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam
update notebook with keywords
0f78bb6 
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam
remove default environments from notebook
11a3a1b 
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam
typo fix
0713ac3 
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam noklam marked this pull request as ready for review November 22, 2023 11:30
@noklam noklam removed the request for review from yetudada November 22, 2023 11:30
astrojuanlu
astrojuanlu reviewed Nov 22, 2023
View reviewed changes
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Is the reordering deliberate? Makes the diff a bit more complicated, is there a commit that only modifies the content?

@noklam
Copy link
Contributor Author

noklam commented Nov 23, 2023

OMG it's native notebook diff
image

@noklam
Copy link
Contributor Author

noklam commented Nov 23, 2023

@astrojuanlu Sorry about that, I have revert the reordering, I will do it in a separate PR instead.

@noklam
add example for standalone config
e766d4e 
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
noklam and others added 2 commits November 23, 2023 22:36
@noklam @stichbury
Apply suggestions from code review
9695601 
Co-authored-by: Jo Stichbury <jo_stichbury@mckinsey.com>
Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>
@noklam @stichbury
Update docs/source/configuration/configuration_basics.md
14b85be 
Co-authored-by: Jo Stichbury <jo_stichbury@mckinsey.com>
Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>
@noklam
fix grammar
8dd86c2 
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam
fix format
483a7c9 
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam
fix format
a3ed825 
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
merelcht
merelcht reviewed Nov 23, 2023
View reviewed changes
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

I left some minor edits, but I think the main thing we maybe need to clarify is whether we are saying a Kedro project has environments or when you use the full Kedro framework it assumes environment. I see @stichbury left a comment about that as well, so maybe good to decide what phrasing we will go for.

docs/source/configuration/advanced_configuration.md Outdated Show resolved Hide resolved
docs/source/configuration/advanced_configuration.md Outdated Show resolved Hide resolved
docs/source/configuration/advanced_configuration.md Outdated Show resolved Hide resolved
docs/source/configuration/advanced_configuration.md Outdated Show resolved Hide resolved
docs/source/configuration/advanced_configuration.md Outdated Show resolved Hide resolved
@noklam
fix grammar
55c812b 
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam
missing plurals
f8a910f 
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam
Copy link
Contributor Author

noklam commented Dec 5, 2023

I left some minor edits, but I think the main thing we maybe need to clarify is whether we are saying a Kedro project has environments or when you use the full Kedro framework it assumes environment. I see @stichbury left a comment about that as well, so maybe good to decide what phrasing we will go for.

I prefer "project" because they are "Project's settings". We used to have "Kedro Framework logging", which are renamed to default logging. Fixed the comments

@merelcht @astrojuanlu

merelcht
merelcht approved these changes Dec 5, 2023
View reviewed changes
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

This looks good to me! and on reviewing this for the second time I also agree "kedro project" is the right term over "kedro framework" 👍

docs/source/configuration/advanced_configuration.md Outdated
For the full list of features, please refer to [configuration_basics](./configuration_basics.md) and [advanced_configuration](./advanced_configuration.md)

### How to use Custom Resolvers with `OmegaConfigLoader`
You can register custom resolvers to use non-primitive types for parmaeters.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
You can register custom resolvers to use non-primitive types for parmaeters.
You can register custom resolvers to use non-primitive types for parameters.

astrojuanlu
astrojuanlu approved these changes Dec 5, 2023
View reviewed changes
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Approved with a couple of tiny nits

@noklam noklam enabled auto-merge (squash) December 7, 2023 10:04
@noklam noklam merged commit 83eed17 into develop Dec 7, 2023
11 checks passed
@noklam noklam deleted the noklam/docs-for-#2971 branch December 7, 2023 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merelcht merelcht merelcht approved these changes

@astrojuanlu astrojuanlu astrojuanlu approved these changes

@stichbury stichbury Awaiting requested review from stichbury

Assignees

@noklam noklam

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@noklam @astrojuanlu @stichbury @merelcht