Skip to content

Support for local assignments#54

Merged
gliargovas merged 51 commits intomainfrom
support-for-local-assignments
Aug 31, 2023
Merged

Support for local assignments#54
gliargovas merged 51 commits intomainfrom
support-for-local-assignments

Conversation

@gliargovas
Copy link
Copy Markdown
Collaborator

As of now, all tests except for test_break pass locally.

@github-actions
Copy link
Copy Markdown

OS:ubuntu-20.04
Mon Aug 28 10:36:13 UTC 2023
Summary: 31/32 tests passed.

@gliargovas gliargovas force-pushed the support-for-local-assignments branch from 8f9c7e2 to aa3d839 Compare August 28, 2023 10:47
@github-actions
Copy link
Copy Markdown

OS:ubuntu-20.04
Mon Aug 28 10:56:19 UTC 2023
Summary: 32/32 tests passed.

@github-actions
Copy link
Copy Markdown

OS:ubuntu-20.04
Tue Aug 29 06:39:42 UTC 2023
Summary: 33/33 tests passed.

@gliargovas gliargovas added ready for review This pull request is ready for review and removed work in progress labels Aug 29, 2023
@gliargovas gliargovas requested a review from angelhof August 29, 2023 07:06
@gliargovas
Copy link
Copy Markdown
Collaborator Author

@angelhof Original test break passes, however, I noticed that commands after the loop will not execute. This behavior is similar to the main branch. We need to resolve this issue, but maybe in a separate PR.

@github-actions
Copy link
Copy Markdown

OS:ubuntu-20.04
Tue Aug 29 12:02:32 UTC 2023
Summary: 33/33 tests passed.

Comment thread parallel-orch/config.py
'PASH_TOP', 'PASH_TOP_LEVEL','RANDOM', 'LOGNAME', 'MACHTYPE', 'MOTD_SHOWN', 'OPTERR', 'OPTIND',
'PPID', 'PROMPT_COMMAND', 'PS4', 'SHELL', 'SHELLOPTS', 'SHLVL', 'TERM', 'UID', 'USER', 'XDG_SESSION_ID'}

SIGNIFICANT_VARS = {'foo', 'bar', 'baz'}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add a TODO to not have significant vars and only have insignificant vars

Comment thread parallel-orch/util.py
return True


def parse_env_string_to_dict(content):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this parsing/comparison complete? Or should we just use diff to check the envs

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am pretty scared of this diff/parse. Let's add a comment that it might be problematic. I don't think regular expressions can correctly parse this file.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can leave it as is for now but down the line we need to consider how to do the diffing properly!

Comment thread parallel-orch/scheduler_server.py Outdated
Copy link
Copy Markdown
Member

@angelhof angelhof left a comment

Choose a reason for hiding this comment

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

@gliargovas No need to resolve all comments before merging if this PR is blocking. Keep some comments/notes and then merge if needed :)

#!/bin/bash

## TODO: Pass speculate flag here instead of separate scripts
# TODO: There is a bug here and parsing of RW dependencies doesn't really work
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is that comment relevant anymore? I would delete it I think

Comment thread parallel-orch/util.py
return True


def parse_env_string_to_dict(content):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am pretty scared of this diff/parse. Let's add a comment that it might be problematic. I don't think regular expressions can correctly parse this file.

Comment thread parallel-orch/util.py
return True


def parse_env_string_to_dict(content):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can leave it as is for now but down the line we need to consider how to do the diffing properly!

Comment thread parallel-orch/partial_program_order.py
Comment thread parallel-orch/run_command.sh Outdated
@github-actions
Copy link
Copy Markdown

OS:ubuntu-20.04
Thu Aug 31 17:05:11 UTC 2023
Summary: 33/33 tests passed.

@gliargovas gliargovas merged commit 983c8b1 into main Aug 31, 2023
@gliargovas gliargovas deleted the support-for-local-assignments branch August 31, 2023 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority Try to resolve this issue before others ready for review This pull request is ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Transfer environment from sandbox to workspace

2 participants