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

FIX #62 - Migrate to 0.16.5 #94

Merged
merged 1 commit into from
Oct 25, 2020
Merged

Conversation

takikadiri
Copy link
Collaborator

@takikadiri takikadiri commented Oct 17, 2020

closes #62 #96

Description

This PR enable the support of kedro 0.16.5

Development notes

The major changes concern the remove of run.py managment. That include,

  • Removing run.py from template
  • Removing run.py related code from then cli
  • Removing run.py related tests from template and cli

Checklist

  • Read the contributing guidelines
  • Open this PR as a 'Draft Pull Request' if it is work-in-progress
  • Update the documentation to reflect the code changes
  • Add a description of this change and add your name to the list of supporting contributions in the CHANGELOG.md file. Please respect Keep a Changelog guidelines.
  • Add tests to cover your changes

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

  • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.

  • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorised to submit this contribution on behalf of the original creator(s) or their licensees.

  • I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

@codecov-io
Copy link

codecov-io commented Oct 17, 2020

Codecov Report

Merging #94 into develop will decrease coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #94      +/-   ##
===========================================
- Coverage    97.45%   97.39%   -0.07%     
===========================================
  Files           32       32              
  Lines         1258     1227      -31     
===========================================
- Hits          1226     1195      -31     
  Misses          32       32              
Impacted Files Coverage Δ
tests/conftest.py 100.00% <ø> (ø)
tests/framework/cli/test_cli.py 100.00% <ø> (ø)
tests/framework/hooks/test_node_hook.py 98.41% <ø> (ø)
kedro_mlflow/framework/cli/cli.py 100.00% <100.00%> (ø)
kedro_mlflow/framework/hooks/__init__.py 100.00% <100.00%> (ø)
kedro_mlflow/framework/hooks/node_hook.py 100.00% <100.00%> (ø)
kedro_mlflow/framework/hooks/pipeline_hook.py 98.79% <100.00%> (+0.01%) ⬆️
kedro_mlflow/utils.py 100.00% <100.00%> (ø)
tests/test_utils.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8f9f09...33e341e. Read the comment docs.

Copy link
Owner

@Galileo-Galilei Galileo-Galilei left a comment

Choose a reason for hiding this comment

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

It's almost ok, just a a few missing updates:

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Owner

@Galileo-Galilei Galileo-Galilei left a comment

Choose a reason for hiding this comment

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

  • very minor typo in the doc
  • a discussion about the opportunity to use kedro get_project_static_data

Otherwise it's good to go.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
docs/source/03_tutorial/02_setup.md Outdated Show resolved Hide resolved
docs/source/03_tutorial/02_setup.md Outdated Show resolved Hide resolved
docs/source/03_tutorial/02_setup.md Outdated Show resolved Hide resolved
docs/source/03_tutorial/02_setup.md Outdated Show resolved Hide resolved
docs/source/03_tutorial/02_setup.md Outdated Show resolved Hide resolved
kedro_version=KEDRO_VERSION,
)


def _get_project_globals_with_pyproject(
Copy link
Owner

Choose a reason for hiding this comment

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

Why don't you use get_static_project_data (from kedro/framework/context/context)? The goal was to avoid redefining thing by ourselves and to default to their implentation. If we can't do this for a specific reason this adds a lot of maintenance issues...

We may just make a try import get_static_project_data, or a check on the kedro version>=0.16.5.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've temporarily copy paste it, waiting for kedro < 0.16.5 support drop. But yes, it's more cleaner to just try import kedro's get_static_project_data and import our _get_project globals as get_static_project_data

Copy link
Owner

Choose a reason for hiding this comment

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

This breaks the test coverage (because only one of the 2 lines of the sucessful import are imported, and the other conditional import is never called in a test). Add "# pragma: no cover" on the lines 15-16, and everything's ok !

Comment on lines 15 to 16
except ImportError:
from kedro_mlflow.utils import _get_project_globals as get_static_project_data
Copy link
Owner

Choose a reason for hiding this comment

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

Add "# pragma: no cover" to avoid decreasing coverage.

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.

Make mlflow init work when configuration is in pyproject.toml Migrate to 0.16.5
3 participants