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.

Sign up for GitHub

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

Jump to bottom

nits: sort imports, doc typos, minor cleanups #661

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

nits: sort imports, doc typos, minor cleanups #661

wants to merge 2 commits into from

Conversation

ryan-williams
Copy link
Contributor

factored out of #612

@ryan-williams ryan-williams mentioned this pull request Aug 24, 2021
Draft
@savingoyal
Copy link
Collaborator

@ryan-williams Thanks for the PR. We would be glad to get this PR and #218 + #323 going once we have merged #580 which is a significant change on master.

@ryan-williams
Copy link
Contributor Author

rebased this, and looks like #580 was merged, so maybe this is ready for another look.

romain-intel
romain-intel reviewed Oct 28, 2021
View reviewed changes
Copy link
Contributor

@romain-intel romain-intel left a comment

Choose a reason for hiding this comment

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

Minor comments. Should be good to go (after running it through black). I just did a review of the code, didn't run it.

metaflow/decorators.py Outdated
kv_regex = r',(?=[\s\w]+=)'
kvs = re.split(kv_regex, quote_stripped)
attrs = dict(
[ x.strip() for x in kv.split('=', 1) ]
Copy link
Contributor

Choose a reason for hiding this comment

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

@savingoyal : I think this is correct but the , 1 is different from the previous code. I think it should work. Not sure why we need the positive lookahead here though (it was present before too)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's this block in the latest commits, fwiw

metaflow/flowspec.py Outdated Show resolved Hide resolved
metaflow/flowspec.py Show resolved Hide resolved
@ryan-williams ryan-williams force-pushed the nits branch 3 times, most recently from fd3ed51 to 4ffc2be Compare October 29, 2021 16:35
@ryan-williams ryan-williams force-pushed the nits branch 3 times, most recently from 4d8314f to d116d04 Compare January 6, 2022 15:02
@ryan-williams
Copy link
Contributor Author

ryan-williams commented Jan 6, 2022 •
edited
Loading

"Python tests on ubuntu-latest" failed, GHA log says:

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 this toward the end of 1_tests Python tests on ubuntu-latest.txt (48M):

2022-01-06T15:30:01.2401382Z [pid 1759] ### The following tests failed: ###
2022-01-06T15:30:01.2402956Z [pid 1759] ### test 'CardTimeoutTest' graph 'simple-foreach' in context python3-all-local ###
2022-01-06T15:30:01.2404842Z [pid 1759] ### test 'CardTimeoutTest' graph 'single-and-branch' in context python3-all-local ###
2022-01-06T15:30:01.2406043Z [pid 1759] ### test 'CardTimeoutTest' graph 'nested-foreach' in context python3-all-local ###
2022-01-06T15:30:01.2407218Z [pid 1759] ### test 'CardTimeoutTest' graph 'nested-branches' in context python3-all-local ###
2022-01-06T15:30:01.2408392Z [pid 1759] ### test 'CardTimeoutTest' graph 'single-linear-step' in context python3-all-local ###
2022-01-06T15:30:01.2409566Z [pid 1759] ### test 'CardTimeoutTest' graph 'small-foreach' in context python3-all-local ###
2022-01-06T15:30:01.2410920Z [pid 1759] ### test 'CardTimeoutTest' graph 'small-parallel' in context python3-all-local ###
2022-01-06T15:30:01.3857245Z ERROR: InvocationError for command /home/runner/work/metaflow/metaflow/test_runner (exited with code 1)
2022-01-06T15:30:01.3860641Z ___________________________________ summary ____________________________________
2022-01-06T15:30:01.3861286Z ERROR:   python: commands failed
2022-01-06T15:30:01.4089259Z ##[error]Process completed with exit code 1.

Not sure what that is about. investigating…

Update: this was due to the black issue fixed in #885

This was referenced Jan 6, 2022
Merged
Closed
@ryan-williams ryan-williams force-pushed the nits branch 2 times, most recently from 6e6df6e to d31bb5a Compare January 7, 2022 19:34
@ryan-williams
Copy link
Contributor Author

I think this is ready for review

@ryan-williams ryan-williams force-pushed the nits branch 2 times, most recently from 74d9efa to f1a8e95 Compare May 25, 2022 17:50
@saikonen
Copy link
Collaborator

going through some older PR's, this seems like a straightforward one to push through. Would you still be available for rebasing it against the latest master ?

@saikonen saikonen added the stale Possibly a stale PR, waiting for author response. label Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romain-intel romain-intel romain-intel left review comments

Assignees
No one assigned
Labels
stale Possibly a stale PR, waiting for author response.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ryan-williams @savingoyal @saikonen @romain-intel