Skip to content

Proc_params for communicating arbitrary object between process and process model#162

Merged
bamsumit merged 9 commits intolava-nc:mainfrom
bamsumit:proc_params
Jan 31, 2022
Merged

Proc_params for communicating arbitrary object between process and process model#162
bamsumit merged 9 commits intolava-nc:mainfrom
bamsumit:proc_params

Conversation

@bamsumit
Copy link
Copy Markdown
Contributor

@bamsumit bamsumit commented Jan 15, 2022 •
edited
Loading

Implemented proc_params in process model constructor and used it for dataloader processes.

Signed-off-by: bamsumit [email protected]

Issue Number:

Objective of pull request:

Pull request checklist

Your PR fulfills the following requirements:

  • Issue created that explains the change and why it's needed
  • Tests are part of the PR (for bug fixes / features)
  • Docs reviewed and added / updated if needed (for bug fixes / features)
  • PR conforms to Coding Conventions
  • PR applys BSD 3-clause or LGPL2.1+ Licenses to all code files
  • Lint (pyb) passes locally
  • Build tests (pyb -E unit) or (python -m unittest) passes locally

Pull request type

Please check your PR type:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation changes
  • Other (please describe):

What is the current behavior?

  • We cannot communicate anything other than PyPorts and Vars between process and process model constructor

What is the new behavior?

  • Process params is available in the process model constructor.
  • New resource type HostCPU to indicate the process model must run on Host CPU.

Does this introduce a breaking change?

  • Yes
  • No

The execution may halt if you have overloaded the process model constructor without proc_params argument. The intended way to overload is

def __init__(self, proc_params):
    super(<YourProcModel>, self).__init__(proc_params)

Supplemental information

@bamsumit bamsumit added the 1-feature New feature request label Jan 15, 2022
@bamsumit bamsumit linked an issue Jan 15, 2022 that may be closed by this pull request
Communication of special objects between process definiton and model definition #159
Closed
7 tasks
@bamsumit bamsumit added the 0-needs-review For all new issues label Jan 15, 2022
@bamsumit bamsumit self-assigned this Jan 19, 2022
mathisrichter
mathisrichter requested changes Jan 25, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@mathisrichter mathisrichter left a comment

Choose a reason for hiding this comment

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

Looks good overall! :)

Biggest concern: Should this PR be split in two to separate the addition of the proc_params and the addition of the dataloader? @mgkwill, what do you think?

Otherwise, I marked a bunch of minor things and suggested refactoring to reduce code duplication.

Comment thread src/lava/magma/compiler/builder.py Outdated
Comment thread tests/lava/proc/io/test_dataloader.py Outdated
Comment thread src/lava/proc/io/dataloader.py Outdated
Comment thread src/lava/magma/core/model/py/model.py Outdated
Comment thread src/lava/proc/io/dataloader.py Outdated
Comment thread tests/lava/proc/io/test_dataloader.py Outdated
Comment thread tests/lava/proc/io/test_dataloader.py Outdated
Comment thread tests/lava/proc/io/test_dataloader.py Outdated
Comment thread tests/lava/proc/io/test_dataloader.py Outdated
Comment thread tests/lava/proc/io/test_dataloader.py
@ashishrao7 ashishrao7 mentioned this pull request Jan 25, 2022
Closed
7 tasks
ashishrao7
ashishrao7 approved these changes Jan 26, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@ashishrao7 ashishrao7 left a comment

Choose a reason for hiding this comment

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

I have not yet gone through the design decisions but I can confirm that with this PR one can communicate arbitrary objects between process and process model. I was successfully able to use this behavior in my setup

@bamsumit bamsumit requested a review from PhilippPlank January 27, 2022 17:04
awintel
awintel requested changes Jan 27, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Looks good overall.
Suggest to clean up the signatures of init and do proper initialization in AbstractProcess and AbstractProcModel.

Comment thread src/lava/magma/compiler/builder.py Outdated
Comment thread src/lava/magma/compiler/builder.py Outdated
Comment thread src/lava/magma/core/model/py/model.py Outdated
Comment thread src/lava/magma/core/model/py/model.py Outdated
Comment thread src/lava/magma/core/model/py/model.py Outdated
@awintel
Copy link
Copy Markdown
Contributor

awintel commented Jan 28, 2022

Don't forget to also close #164 when this gets pushed.

@PhilippPlank PhilippPlank linked an issue Jan 28, 2022 that may be closed by this pull request
Accessing Non-lava type process attributes in process models #164
Closed
7 tasks
@bamsumit
Made proc_params required argument for AbstractProcessModel and all i…
72fde72 
…t's derivatives. Fixed all the broken code in the current repo because of it.

Signed-off-by: bamsumit <[email protected]>
@bamsumit
Copy link
Copy Markdown
Contributor Author

bamsumit commented Jan 28, 2022

Don't forget to also close #164 when this gets pushed.

It's already linked to this PR.

@bamsumit bamsumit requested a review from awintel January 28, 2022 23:38
awintel
awintel approved these changes Jan 29, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Looks good. But could you also update the AbstractProcess docstring (as requested by Ashish, so people know they can set proc_params in their init method?
Ideally this should also be documented in the ProcModel docstring but this one is currently outdated already.

mathisrichter
mathisrichter approved these changes Jan 30, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@mathisrichter mathisrichter left a comment

Choose a reason for hiding this comment

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

Great work! :)

Comment thread src/lava/proc/io/dataloader.py Outdated
Comment thread src/lava/proc/io/dataloader.py Outdated
@bamsumit
Update to docstrings.
bcfabc3 
Signed-off-by: bamsumit <[email protected]>
@bamsumit bamsumit merged commit 364085d into lava-nc:main Jan 31, 2022
@bamsumit bamsumit deleted the proc_params branch January 31, 2022 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mathisrichter mathisrichter mathisrichter approved these changes

@ashishrao7 ashishrao7 ashishrao7 approved these changes

@awintel awintel awintel approved these changes

@PhilippPlank PhilippPlank Awaiting requested review from PhilippPlank

+1 more reviewer

@joyeshmishra joyeshmishra joyeshmishra approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@bamsumit bamsumit

Labels

0-needs-review For all new issues 1-feature New feature request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Accessing Non-lava type process attributes in process models Communication of special objects between process definiton and model definition

5 participants

@bamsumit @awintel @joyeshmishra @mathisrichter @ashishrao7