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

change extension in pipeline.yaml when changing scripts/notebooks format #755

Merged
merged 10 commits into from
May 18, 2022

Conversation

idomic
Copy link
Contributor

@idomic idomic commented May 12, 2022

As part of the inner task extension format, for each change we're editing the pipeline.yaml file

@idomic
Copy link
Contributor Author

idomic commented May 12, 2022

A bit more efficient solution can be reiterating on the changed tasks after change and then reading/writing to the file once, but then we have to maintain the state of the old extensions for changed files.

@@ -606,6 +606,9 @@ def format(self, fmt):
if extension_changed:
path = self._path.with_suffix(ext_format)
Path(self._path).unlink()
modified_entry = (Path(entry_point).read_text().replace(
Copy link
Contributor

Choose a reason for hiding this comment

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

there's an edge case here. we also support other types of entry points, for example, it might be a path_to_module.path_to_function string so read_text() will fail in that case.

I suggest checking Path(entry_point).is_file(), if False, we should print a message with click.secho (in yellow font) saying that the entry point is not a file, and skip the extension replacement - the user will have to do it manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Functions doesn't have format so it doesn't even gets to this code, it throws an exception
Out[1]: PythonCallableSource(function) (defined at: '/private/var/folders/qx/px2tf0rn5bq6c79yd3gk37fwzpvmb9/T/tmpgnur3y2z/content/my_tasks/raw/functions.py:4')
'PythonCallableSource' object has no attribute 'format' We can either implement it with the secho or print it as part of the except block

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't mean a function which is a task in a pipeline. I meant an entry point. We support users defining a function that returns a DAG object. Example:

ploomber build --entry-point some_module.some_function

In such case, the code will break because entry_point isn't a file.

Example here: https://docs.ploomber.io/en/latest/user-guide/parametrized.html#Python-API-(factory-functions)

src/ploomber/sources/notebooksource.py Outdated Show resolved Hide resolved
@edublancas edublancas closed this May 14, 2022
@edublancas edublancas deleted the nb_pipeline_change_624 branch May 14, 2022 23:39
@edublancas edublancas restored the nb_pipeline_change_624 branch May 14, 2022 23:40
@edublancas edublancas reopened this May 14, 2022

def test_format_no_entry_point(monkeypatch, tmp_nbs_factory, capsys):
monkeypatch.setattr(sys, 'argv', [
'ploomber', 'nb', '--entry-point', 'factory.make', '--format', 'ipynb'
Copy link
Contributor

Choose a reason for hiding this comment

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

the CI is failing. I think if you replace factory.make for test_pkg.entry.with_doct it should work since that function is already defined

Copy link
Contributor

Choose a reason for hiding this comment

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

I think factory.make isn't working because the module isn't in the path. so if you go with test_pkg you can delete the nbs-factory code and fixture.

@edublancas edublancas changed the title Initial commit change extension in pipeline.yaml when changing scripts/notebooks format May 18, 2022
@edublancas edublancas merged commit 79de741 into master May 18, 2022
@edublancas edublancas deleted the nb_pipeline_change_624 branch May 18, 2022 20:46
neelasha23 pushed a commit to neelasha23/ploomber that referenced this pull request May 22, 2022
…mat (ploomber#755)

* Initial commit

* Testing pipeline doesn't change

* Fixing tests

* minor changes

* Added edge tests

* lints

* Fixing test

* Fixing test

* fixing test

Co-authored-by: Eduardo Blancas Reyes <github@blancas.io>
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.

None yet

2 participants