Skip to content

Conversation

@ketangangal
Copy link
Contributor

@ketangangal ketangangal commented May 24, 2022

Describe changes

I fixed issue : Log a warning instead of raising an AssertionError

else:
    # TODO [ENG-895]: Refactor this to raise a warning instead.
    raise AssertionError(
        "Cannot set magic flag in non-notebook environments."
    )

to

else:
    cli_utils.warning(
        "Cannot set magic flag in non-notebook environments."
    )

@strickvl strickvl changed the base branch from main to develop May 24, 2022 07:28
@strickvl strickvl changed the title fix: Log a warning instead of raising an AssertionError FIX: Log a warning instead of raising an AssertionError May 24, 2022
@strickvl strickvl added enhancement New feature or request good first issue labels May 24, 2022
Copy link
Contributor

@strickvl strickvl left a comment

Choose a reason for hiding this comment

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

Thanks for these suggested fixes, @ketangangal!

Some small changes still on this, since the imports weren't quite correct.

…isualizer.py

Co-authored-by: Alex Strick van Linschoten <strickvl@users.noreply.github.com>
@strickvl
Copy link
Contributor

@ketangangal just wanted to let you know that you can just update your PR from within this one. No real need to create a new one. It helps keep all the work that was done in one place. We can then see how things developed as a result of feedback etc.

@strickvl strickvl reopened this May 24, 2022
@ketangangal
Copy link
Contributor Author

@strickvl got it thanks correcting mistakes.

Copy link
Contributor

@strickvl strickvl left a comment

Choose a reason for hiding this comment

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

One other thing I just remembered from #616 is that the imports should probably be as in my suggested change (i.e. cli_utils.warning and the import change as suggested).

The way you did it before was technically correct, but it just didn't fit how we do it usually in the codebase.

@ketangangal ketangangal requested a review from strickvl May 24, 2022 09:18
@ketangangal
Copy link
Contributor Author

@strickvl done !

@strickvl
Copy link
Contributor

strickvl commented May 24, 2022

@strickvl done !

@ketangangal You can see in the CI testing that we have a bunch of failures there. Looking at the error for one of those, it says that the imports are unsorted:

ERROR: /Users/runner/work/zenml/zenml/src/zenml/integrations/dash/visualizers/pipeline_run_lineage_visualizer.py Imports are incorrectly sorted and/or formatted.

I think you didn't run the Poetry linting script as suggested in the CONTRIBUTING.md file. I'd suggest you do that and resubmit. Then we can make sure it's really passing our tests.

@strickvl strickvl linked an issue May 25, 2022 that may be closed by this pull request
@strickvl
Copy link
Contributor

strickvl commented Jun 2, 2022

@ketangangal I took the liberty of running the linting script on your behalf. If the tests pass I'll be sure to merge this in. Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE]: Log a warning instead of raising an AssertionError

4 participants