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

Bugfix to enable monitoring aliased vars #675

Merged
merged 7 commits into from
May 2, 2023
Merged

Bugfix to enable monitoring aliased vars #675

merged 7 commits into from
May 2, 2023

Conversation

AlessandroPierro
Copy link
Contributor

@AlessandroPierro AlessandroPierro commented Apr 27, 2023

Issue Number: #676

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 (flakeheaven lint src/lava tests/) and (bandit -r src/lava/.) pass locally
  • Build tests (pytest) passes locally

Pull request type

Please check your PR type:

  • Bugfix

What is the current behavior?

  • An assertion was raised when it was requested to create two different implicit var ports with the same name on the same process

What is the new behavior?

  • The different var ports will be created without assertion, with a numerical suffix added to the name

Does this introduce a breaking change?

  • No

@AlessandroPierro
Copy link
Contributor Author

Required to merge: lava-nc/lava-optimization#217

@PhilippPlank PhilippPlank self-assigned this Apr 27, 2023
@PhilippPlank PhilippPlank added the 1-bug Something isn't working label Apr 27, 2023
@PhilippPlank PhilippPlank linked an issue Apr 27, 2023 that may be closed by this pull request
Copy link
Contributor

@weidel-p weidel-p left a comment

Choose a reason for hiding this comment

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

This PR adds a suffix to a port name if it already exists which is an easy solution to the problem described in the issue.

I have one request though, could you please add a test which shows that the original issue is solved? So that we can probe aliased vars?

src/lava/magma/core/process/ports/ports.py Outdated Show resolved Hide resolved
@PhilippPlank PhilippPlank merged commit ae45028 into lava-nc:main May 2, 2023
@AlessandroPierro AlessandroPierro deleted the bugfix-monitor-aliased-var branch May 2, 2023 10:01
@tim-shea tim-shea added this to the Lava v0.7.1 milestone May 15, 2023
monkin77 pushed a commit to monkin77/thesis-lava that referenced this pull request Jul 12, 2024
* Bugfix to enable monitoring aliased vars

* Update ports.py

* Update ports.py

* Update test_ports.py

* Update test_ports.py

* Address review feedback

* Match expected behaviour for implicit var port naming

---------

Co-authored-by: PhilippPlank <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1-bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to probe aliased vars with Monitor
4 participants