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

Refactor Message Infrastructure #29

Merged
merged 7 commits into from
Nov 11, 2021
Merged

Refactor Message Infrastructure #29

merged 7 commits into from
Nov 11, 2021

Conversation

joyeshmishra
Copy link
Contributor

No description provided.

@awintel awintel requested a review from jlakness November 5, 2021 18:57
awintel
awintel requested changes Nov 5, 2021
View reviewed changes
Copy link
Contributor

@awintel awintel left a comment

Choose a reason for hiding this comment

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

Thanks. Looks good overall. Feedback is mostly about formatting and consistency.

However, there is one bug around set_var/get_var that should be looked at... It seems that this won't work right now when 'idx' is provided.

lava/magma/core/process/process.py Outdated Show resolved Hide resolved
lava/magma/runtime/message_infrastructure/message_infrastructure_interface.py Outdated Show resolved Hide resolved
lava/magma/runtime/message_infrastructure/message_infrastructure_interface.py Outdated Show resolved Hide resolved
lava/magma/runtime/message_infrastructure/message_infrastructure_interface.py Outdated Show resolved Hide resolved
lava/magma/runtime/message_infrastructure/multiprocessing.py Outdated Show resolved Hide resolved
lava/magma/runtime/runtime.py Outdated Show resolved Hide resolved
lava/magma/runtime/runtime.py Outdated Show resolved Hide resolved
tests/magma/runtime/test_runtime.py Outdated Show resolved Hide resolved
lava/magma/compiler/channels/pypychannel.py Outdated Show resolved Hide resolved
lava/magma/compiler/channels/pypychannel.py Outdated Show resolved Hide resolved
@mgkwill
Copy link
Contributor

mgkwill commented Nov 10, 2021

Source now moved from /lava to /src/lava.
Tests now moved from /tests to /tests/lava.

I'll need to push an update to fix this before we can merge.

mgkwill
mgkwill approved these changes Nov 10, 2021
View reviewed changes
Copy link
Contributor

@mgkwill mgkwill left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@awintel @jlakness-intel I've merged latest into this and resolved most of Andreas left over issues that were not too involved.

We should probably have @joyeshmishra or one of us address further issues in a future change:

  1. Refactor Message Infrastructure #29 (comment)
  2. Refactor Message Infrastructure #29 (comment)
  3. Refactor Message Infrastructure #29 (comment)
  4. https://github.com/lava-nc/lava/pull/29/files/67288bde232bf2a92f7a7c98f2de9780718bfa01#r743942155
  5. https://github.com/lava-nc/lava/pull/29/files/67288bde232bf2a92f7a7c98f2de9780718bfa01#r743994391
  6. https://github.com/lava-nc/lava/pull/29/files/67288bde232bf2a92f7a7c98f2de9780718bfa01#r744005895
  7. https://github.com/lava-nc/lava/pull/29/files/67288bde232bf2a92f7a7c98f2de9780718bfa01#r744005534

Per the conversation at WG do we want to merge this? We can then base all inflight code on it.

@mgkwill
Fix some review issues
8fbf45c 
Signed-off-by: Marcus G K Williams <[email protected]>
@mgkwill mgkwill self-assigned this Nov 10, 2021
@mgkwill mgkwill added 1-feature New feature request reviewed-approved labels Nov 10, 2021
@awintel
Copy link
Contributor

awintel commented Nov 11, 2021

I think it's fine to merge. Apparently none of the existing unit tests broke. The bug that I pointed out in the way set/get is implemented can also be addresses as part of a new bug issue after merging this one to main.

@mgkwill
Copy link
Contributor

mgkwill commented Nov 11, 2021

I think it's fine to merge. Apparently none of the existing unit tests broke. The bug that I pointed out in the way set/get is implemented can also be addresses as part of a new bug issue after merging this one to main.

@awintel can you approve to enable merge?

@awintel awintel removed the request for review from jlakness November 11, 2021 02:10
@mgkwill mgkwill merged commit 33b3fcc into main Nov 11, 2021
@mgkwill mgkwill deleted the refact branch November 11, 2021 02:27
monkin77 pushed a commit to monkin77/thesis-lava that referenced this pull request Jul 12, 2024
@joyeshmishra
Refactor Message Infrastructure (lava-nc#29)
1f04f8d 
* Refactor Message Infrastructure
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgkwill mgkwill mgkwill approved these changes

@awintel awintel awintel approved these changes

@PhilippPlank PhilippPlank Awaiting requested review from PhilippPlank

Assignees

@mgkwill mgkwill

Labels
1-feature New feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@joyeshmishra @mgkwill @awintel