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

error when finding pipeline.yaml in root directory #497

Closed
edublancas opened this issue Jan 22, 2022 · 11 comments
Closed

error when finding pipeline.yaml in root directory #497

edublancas opened this issue Jan 22, 2022 · 11 comments
Assignees
Labels
good first issue Good for newcomers

Comments

@edublancas
Copy link
Contributor

the recursive function looks at .parent[0] but the root directory does not have a parent

@edublancas
Copy link
Contributor Author

the problem is here:

if root_by_pipeline.parents[0].name == 'src':

there are a bunch of places where we do parents[0] but if the current working directory is the root of the filesystem, parents[0] will throw an exception

@edublancas edublancas added the good first issue Good for newcomers label Jan 31, 2022
@idomic
Copy link
Contributor

idomic commented Mar 16, 2022

@arturomf94 wanna take this?

@nph4rd
Copy link
Contributor

nph4rd commented Mar 22, 2022

@arturomf94 wanna take this?

Sorry @idomic - I unfortunately can't atm. Will see if this is still open when I've got a bit more time to contribute!

@idomic
Copy link
Contributor

idomic commented Mar 23, 2022

@neelasha23 This one is also open!

@neelasha23
Copy link
Contributor

Sure! Will take a look

@neelasha23
Copy link
Contributor

neelasha23 commented Mar 27, 2022

@edublancas
To clarify, this means on a Mac, the path would be '/pipeline.yaml' ?
I tried to write a sample test case here : (https://github.com/ploomber/ploomber/blob/master/tests/util/test_default.py) to reproduce the error by doing something like:

pip = Path('/pipeline.yaml')
pip.touch()

however, this throws error : OSError: [Errno 30] Read-only file system: '/pipeline.yaml'

I referred this article as well: https://sw0zwl.medium.com/how-to-create-a-directory-in-root-on-catalina-or-big-sur-e58e459461a, but this allows us to create directory at the root and not a file. Any suggestions on how to go about this issue?

@idomic
Copy link
Contributor

idomic commented Mar 27, 2022

Yeah in mac/linux / is the root, I think it'll be easier for you to mock the location, look on this test:
tests/spec/test_dagspec.py:682
You can create the file in the mock location. I don't think the os will let you edit files on the root level. Let's see if Eduardo has ways to reproduce.

@neelasha23
Copy link
Contributor

@idomic Okay, checking this testcase

@edublancas
Copy link
Contributor Author

hi @neelasha23, thanks for looking into this. yes, the way to go is to mock the pipeline location.

so the error will be raised here:

if root_by_pipeline.parents[0].name == 'src':

we need to mock find_parent_of_file_recursively to that it returns the root folder /:

something like this (not sure if this will actually run), but it'll illustrate the point:

from pathlib import Path
from unittest.mock import Mock
from ploomber.util import default

def test_pipeline_yaml_in_root_directory(monkeypatch):
    root = Path('/')
    mock = Mock(return_value=(root, None)) 
    # mock
    monkeypatch.setattr(default, 'find_parent_of_file_recursively', mock)

   # now we test
   find_root_recursively()

monkeypatch reference: https://docs.pytest.org/en/6.2.x/monkeypatch.html

This should throw an error because the root directory / doesn't have parents. Let me know if you're able to reproduce this, then, please add try ... catch logic to provide a more meaningful message like "pipeline.yaml cannot be in the root directory, please add it inside a directory like project-name/pipeline.yaml"

@neelasha23
Copy link
Contributor

neelasha23 commented Mar 29, 2022

@edublancas @idomic
Thanks a lot for the guidance.

This is the test case that raises IndexError originally:

def test_pipeline_yaml_in_root_directory(monkeypatch):
    root = Path(os.path.abspath(os.sep))
    #setup.py root is None but pipeline.yaml root is filesystem root
    return_data = [(None, None), (root, None)]
    mock = Mock(side_effect=return_data)
    monkeypatch.setattr(default, 'find_parent_of_file_recursively', mock)
    with pytest.raises(DAGSpecInvalidError) as excinfo:
        default.find_root_recursively()

    assert (
            'pipeline.yaml cannot be in the root directory, '
            'please add it inside a directory like project-name/pipeline.yaml') in str(excinfo.value)
    assert mock.call_count == len(return_data)

I can include two other similar test cases for all the 3 places in https://github.com/ploomber/ploomber/blob/master/src/ploomber/util/default.py where if root_by_pipeline.parents[0].name == 'src': gets invoked.

Also, in the default.py file just after the root_by_pipeline, pipeline_levels = find_parent_of_file_recursively(..) call I have added:

    if root_by_pipeline == Path(os.path.abspath(os.sep)):
        raise DAGSpecInvalidError(
            'pipeline.yaml cannot be in the root directory, '
            'please add it inside a directory like project-name/pipeline.yaml')

If this doesnt look right I will add try catch block instead as suggested:

try:
 ...
except IndexError:
  raise DagSpecInvalidError(...)

@edublancas
Copy link
Contributor Author

fixed by #695

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants