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 black . issue #885

Merged
merged 1 commit into from Jan 7, 2022
Merged

fix black . issue #885

merged 1 commit into from Jan 7, 2022

Conversation

ryan-williams
Copy link
Contributor

Rebasing #612 and friends, black . is showing this unrelated change. I folded it into #661 but the CI is failing there for a reason I'm not sure is due to my changes, so I'm submitting this separately partly to see if I see the same failure here.

@ryan-williams
Copy link
Contributor Author

ryan-williams commented Jan 6, 2022

This GHA also failed, this time in "Python tests on macos-latest" (logs again to big to view in browser):

This step has been truncated due to its large size. Download the full logs from the  menu once the workflow run has completed.

I downloaded the logs and see the same info about CardTimeoutTest failing:

2022-01-06T16:24:53.3866230Z [pid 2730] ### The following tests failed: ###
2022-01-06T16:24:53.3867780Z [pid 2730] ### test 'CardTimeoutTest' graph 'simple-foreach' in context python3-all-local ###
2022-01-06T16:24:53.3869330Z [pid 2730] ### test 'CardTimeoutTest' graph 'nested-branches' in context python3-all-local ###
2022-01-06T16:24:53.3870760Z [pid 2730] ### test 'CardTimeoutTest' graph 'small-foreach' in context python3-all-local ###
2022-01-06T16:24:53.3872190Z [pid 2730] ### test 'CardTimeoutTest' graph 'nested-foreach' in context python3-all-local ###
2022-01-06T16:24:53.3873620Z [pid 2730] ### test 'CardTimeoutTest' graph 'small-parallel' in context python3-all-local ###
2022-01-06T16:24:53.3875050Z [pid 2730] ### test 'CardTimeoutTest' graph 'single-and-branch' in context python3-all-local ###
2022-01-06T16:24:53.3876570Z [pid 2730] ### test 'CardTimeoutTest' graph 'single-linear-step' in context python3-all-local ###
2022-01-06T16:24:53.5299110Z ERROR: InvocationError for command /Users/runner/work/metaflow/metaflow/test_runner (exited with code 1)
2022-01-06T16:24:53.5310040Z ___________________________________ summary ____________________________________
2022-01-06T16:24:53.5310730Z ERROR:   python: commands failed
2022-01-06T16:24:53.5653260Z ##[error]Process completed with exit code 1.

Searching for CardTimeoutTest, I see many blocks like this:

2022-01-06T16:03:05.8699650Z [pid 2736] ### Loaded test CardTimeoutTest ###
2022-01-06T16:03:05.8803130Z [pid 2736] ### test 'CardTimeoutTest' graph 'simple-foreach' in context 'python3-all-local': running ###
2022-01-06T16:03:05.9005740Z Metaflow 2.4.7+test executing BasicParameterTestFlow for user:tester
2022-01-06T16:03:05.9108010Z   File "test_flow.py", line 13
2022-01-06T16:03:05.9209760Z     step_start(self):
2022-01-06T16:03:05.9229920Z                     ^
2022-01-06T16:03:05.9239680Z SyntaxError: invalid syntax

@ryan-williams
Copy link
Contributor Author

It seems like the change by black . messes up the generation of the test_flow.py:

# -*- coding: utf-8 -*-
from metaflow import FlowSpec, step, Parameter, project, IncludeFile, JSONType, current, parallel
from metaflow_test import assert_equals, assert_exception, ExpectationFailed, is_resumed, ResumeFromHere, TestRetry
from metaflow import card

class CardTimeoutTestFlow(FlowSpec):
    @card(type="test_timeout_card",timeout=10,options=dict(timeout=20),save_errors=False)
    @step
    def start(self):
        'card(type="test_timeout_card",timeout=10,options=dict(timeout=20),save_errors=False)'

        ps(0, ["start"])
        step_start(self):
        from metaflow import current

        self.task = current.pathspec
        self.next(self.foreach_split)
    @step
    def foreach_split(self):
        self.arr = [1, 2, 3]
        pass
        self.next(self.foreach_inner_first, foreach="arr")
    @step
    def foreach_inner_first(self):
        pass
        self.next(self.foreach_inner_second)
    @step
    def foreach_inner_second(self):
        pass
        self.next(self.foreach_join)
    @step
    def foreach_join(self, inputs):
        pass
        self.next(self.end)
    @step
    def end(self):
        pass
if __name__ == '__main__':
    CardTimeoutTestFlow()

No idea what is going on there, changed this PR to just have black ignore this line

@@ -10,6 +10,7 @@ class CardTimeoutTest(MetaflowTest):

PRIORITY = 2

# fmt: off
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add the equivalent # fmt: on after L:14?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, didn't realize that was necessary, sry

looks like # fmt: skip at end of line now works (psf/black#1800) so I did that, lmk what you think

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks!

Copy link
Collaborator

@savingoyal savingoyal left a comment

Choose a reason for hiding this comment

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

LGTM

@savingoyal savingoyal merged commit 1b2980c into Netflix:master Jan 7, 2022
@ryan-williams ryan-williams deleted the black branch January 8, 2022 01:19
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