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

Change starters to use OmegaConfigLoader #145

Merged
merged 8 commits into from
Aug 30, 2023
Merged

Change starters to use OmegaConfigLoader #145

merged 8 commits into from
Aug 30, 2023

Conversation

lrcouto
Copy link
Contributor

@lrcouto lrcouto commented Aug 23, 2023 •
edited
Loading

Motivation and Context

Part of the resolution for Make starters use OmegaConfigLoader. Changes on the kedro repo can be seen on this PR.

How has this been tested?

Tested on gitpod workspace running make test, and by creating a new Kedro project with a starter.

Checklist

  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Assigned myself to the PR
  • Added tests to cover my changes

@noklam
Copy link
Contributor

noklam commented Aug 23, 2023

Linting is failing - you may want to try to do a kedro new and run kedro lint to see what's wrong.

You can do kedro new --checkout <branch> to use starters that are in feature branch.

@noklam noklam linked an issue Aug 25, 2023 that may be closed by this pull request
Make starters use OmegaConfigLoader #143
Closed
@noklam noklam mentioned this pull request Aug 25, 2023
Closed
@noklam
Copy link
Contributor

noklam commented Aug 25, 2023 •
edited
Loading

/tmp/tmp53i8letx/project-dummy/src/project_dummy/settings.py:25:1: E402 module level import not at top of file

If you read the log or try to run it locally you will see this error. I think we need to suppress the linting, and in 0.19 the default should be just OmegaConfigLoader.

@lrcouto
Copy link
Contributor Author

lrcouto commented Aug 25, 2023

/tmp/tmp53i8letx/project-dummy/src/project_dummy/settings.py:25:1: E402 module level import not at top of file

If you read the log or try to run it locally you will see this error. I think we need to suppress the linting, and in 0.19 the default should be just OmegaConfigLoader.

Yeah I see it now. Do you suppress it with #fmt: on/off for Black or is there something else that needs to be done?

@noklam
Copy link
Contributor

noklam commented Aug 25, 2023

@lrcouto https://www.flake8rules.com/rules/E402.html - the warning should be coming from flake, which is usually with inline comment # noqa: E402. You can search for noqa in the code base to find similar example.

@lrcouto
Copy link
Contributor Author

lrcouto commented Aug 25, 2023

@lrcouto https://www.flake8rules.com/rules/E402.html - the warning should be coming from flake, which is usually with inline comment # noqa: E402. You can search for noqa in the code base to find similar example.

Thank you!

noklam
noklam reviewed Aug 25, 2023
View reviewed changes
databricks-iris/{{ cookiecutter.repo_name }}/src/{{ cookiecutter.python_package }}/settings.py Outdated
@@ -3,7 +3,7 @@
https://kedro.readthedocs.io/en/stable/kedro_project_setup/settings.html."""

# Instantiated project hooks.
from {{cookiecutter.python_package}}.hooks import SparkHooks
from {{cookiecutter.python_package}}.hooks import SparkHooks # noqa: import-outside-toplevel
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you try to run this locally? I am not sure if using the name of the rule works.

We used a few linter and they are not so consistent.

pylint use # pylint: disable=<rule_number> or <rule_name> but flake use # noqa: <rule_number>, AFAIK it doesn't work with the name but please check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to the rule number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I've found the error.

@lrcouto lrcouto mentioned this pull request Aug 25, 2023
Merged
6 tasks
@lrcouto lrcouto marked this pull request as ready for review August 25, 2023 18:34
@lrcouto lrcouto self-assigned this Aug 25, 2023
@noklam noklam self-requested a review August 25, 2023 22:11
@astrojuanlu
Copy link
Member

Tests failing because of PyYAML and Cython 3 yaml/pyyaml#601 (comment)

#144 updated the PyYAML upper version cap, so hopefully updating this branch should suffice.

astrojuanlu
astrojuanlu reviewed Aug 28, 2023
View reviewed changes
.circleci/config.yml Outdated
@@ -58,13 +58,13 @@ commands:
description: Run `kedro run` end to end tests for all starters
steps:
- run:
command: behave features/run.feature
command: behave features/run.feature --summary
Copy link
Member

Choose a reason for hiding this comment

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

Is this part of the starters changes, or is there some other motivation?

Copy link
Contributor

@noklam noklam Aug 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I added this initially because the e2e test log is too noisy and make it hard to see the actual error, this doesn't seem to help so I will revert this

@lrcouto lrcouto force-pushed the change-starters-to-use-omegaconfigloader branch 2 times, most recently from 280215b to ee0e108 Compare August 28, 2023 18:35
noklam
noklam reviewed Aug 29, 2023
View reviewed changes
...-airflow-iris/{{ cookiecutter.repo_name }}/src/{{ cookiecutter.python_package }}/settings.py Outdated
@@ -22,8 +22,9 @@
# CONF_SOURCE = "conf"

# Class that manages how configuration is loaded.
# from kedro.config import OmegaConfigLoader
# CONFIG_LOADER_CLASS = OmegaConfigLoader
from kedro.config import OmegaConfigLoader # noqa: E402
Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/kedro-org/kedro-starters/pull/145/files I see that using the rule name works, do you want to apply the same change here?

lrcouto and others added 7 commits August 29, 2023 12:30
@lrcouto
Change starters to use OmegaConfigLoader
a02a518 
Signed-off-by: lrcouto <laurarccouto@gmail.com>
@lrcouto
Fix linter error
459f513 
Signed-off-by: lrcouto <laurarccouto@gmail.com>
@noklam @lrcouto
Update config.yml
95952d5 
Signed-off-by: lrcouto <laurarccouto@gmail.com>
@lrcouto
Suppress linting for imports outside top level
edecf27 
Signed-off-by: L. R. Couto <laurarccouto@gmail.com>
Signed-off-by: lrcouto <laurarccouto@gmail.com>
@lrcouto
Change flake8 rule suppression to refer to rule number instead of name
9ac490d 
Signed-off-by: lrcouto <laurarccouto@gmail.com>
@lrcouto
Fix linter error regarding number of spaces in inline comment
774b3a5 
Signed-off-by: lrcouto <laurarccouto@gmail.com>
@noklam @lrcouto
Revert "Update config.yml"
ac4facb 
This reverts commit c75a081.

Signed-off-by: lrcouto <laurarccouto@gmail.com>
@lrcouto lrcouto force-pushed the change-starters-to-use-omegaconfigloader branch from 5a38521 to ac4facb Compare August 29, 2023 15:31
@lrcouto
Change flake8 rule ignore to use rule name
7448b90 
Signed-off-by: lrcouto <laurarccouto@gmail.com>
@noklam noklam self-requested a review August 29, 2023 16:08
merelcht
merelcht approved these changes Aug 30, 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.

LGTM! 👍

@astrojuanlu astrojuanlu merged commit 8b5dd09 into main Aug 30, 2023
1 check passed
@astrojuanlu astrojuanlu deleted the change-starters-to-use-omegaconfigloader branch August 30, 2023 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@astrojuanlu astrojuanlu astrojuanlu left review comments

@merelcht merelcht merelcht approved these changes

@noklam noklam noklam approved these changes

@ankatiyar ankatiyar Awaiting requested review from ankatiyar

Assignees

@lrcouto lrcouto

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make starters use OmegaConfigLoader
4 participants
@lrcouto @noklam @astrojuanlu @merelcht