Dashboard: update python parser #1140
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
proy30
changed the title
will need it's own file to handle all the capabilities
make more readable and clearly layout its purpose
will need to handle variable references
not yet completed
does not affect performance, simpler to read
for 'self. calls
we just need the numerical number and not the brackets
do a greedy search of the parenthesis to stop at the last ')' in the line. We need this because sim.extend() can have a element.(..) stored inside, and stopped at the first ')' is premature
replace_variables_to_elements -> replace_variables
update how lattice element counts are stored add lattice stats test expand on testdata/examples cleanup/fix make imports relative fix add tests simplify simplify parse_list_inputs more comments/docstring simplify more cleanup tests: only keep 1-2 hardcoded tests to validate python parser
found the culprit - the built in .reverse() that was being used emitted a reversed copy into the lattice instead of mutating the variable itself, and caused an extra set of elements to appear. we needed the reversed copy to be the only thing pushed to the elements, not the reversed copy AND the non-reversed
proy30
force-pushed
the
dashboard/update_python_parser
branch
from
1c7e153 to
8569ccb
Compare
proy30
changed the title
modify test_python_import's lattice section testing test for correct population of element names and a single element parameter's value I believe this is sufficient enough for testing how the lattice populates when importing from a file fix test_lattice_stats and cleanup fix examples show examples cleanup [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci improvements
proy30
force-pushed
the
dashboard/update_python_parser
branch
from
9b09ad1 to
3ea0c92
Compare
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Thanks a lot for this PR! When I tried to pull this and initialize dashboard I received the attached error--it appears to be related to the previous PR #876. Did you encounter this at any point?
Note: proy30 is currently working to address this, and this PR shouldn't be merged until then.
this funciton is needed by both the source code and the testing files. unsure where a good single location for the function would be without any import issues.
for more information, see https://pre-commit.ci
| numeric_input = GeneralFunctions.convert_to_numeric(current_input) | ||
| setattr(state, state_name, numeric_input) | ||
|
|
||
| def get_impactx_root_dir() -> Path | None: |
Check notice
Code scanning / CodeQL
First parameter of a method is not named 'self' Note
|
Hi @cemitch99 , would you please be able to try again? I was unable to replicate my environment as yours, so I am not fully positive if the solution is now fixed or not. |
|
Hi @proy30 . Thanks for the recent changes! I tried again, using However, if I launch the dashboard using |
|
@cemitch99 Thank you for letting me know. I have to investigate the issue further and will update again. |
|
Fix in #1179 |
| current_directory = Path(__file__).resolve() | ||
| root_dir = None | ||
|
|
||
| for parent_dir in current_directory.parents: |
There was a problem hiding this comment.
@ax3l Thank you for the ping. Yes, there is a duplicate function in both this PR (#1140) and #876.
At first, this was done intentionally when added in because (trying to recall correctly) it was difficult to have the pytests (from the directory impactx/tests/python/dashboard/utils.py call to the function in directory impactx/src/python/impactx/dashboard/Input/utils.py.
Any suggestions for where the function get_impactx_root_dir should be placed? It needs to be accessed by both the dashboard and the dashboard tests. Unsure if, in the future, the functionality could be used elsewhere where the root impactx directory is needed.
| if parent_dir.name == "impactx" and (parent_dir / ".git").is_dir(): | ||
| root_dir = parent_dir # keep going until we reach the highest match |
There was a problem hiding this comment.
@proy30 it looks to me like you are quite often assuming that ImpactX is used from its source tree. This is, outside of development, not the case.
We install ImpactX like this:
cmake -S . -B build -DImpactX_PYTHON=ON
cmake --build build -j 4 --target pip_installwhich copies things outside of the source tree. Thus, looking for a .git/ directory is not a logic that will work when ImpactX was installed/deployed/is used in production.
There was a problem hiding this comment.
@ax3l Thank you for pointing this out. Will add a commit to work well in both development and non-development
There was a problem hiding this comment.
Will utilise the old path and logic from get_impactx_path() that you mention in the comment below.
| Retrieve the selected ImpactX example file and populate the UI with its values. | ||
| """ | ||
|
|
||
| impactx_directory = DashboardExamplesLoader.get_impactx_path() |
There was a problem hiding this comment.
Keep using the old path and logic from get_impactx_path(), I fixed the logic in #1179
| for parent_dir in current_directory.parents: | ||
| if parent_dir.name == "impactx" and (parent_dir / ".git").is_dir(): | ||
| root_dir = parent_dir # keep going until we reach the highest match | ||
| return root_dir |
There was a problem hiding this comment.
See above, please don't add this .git/ path based logic.
It should work if you rely on the one I fixed in #1179
|
@proy30 let me know when you are ready for me to review the updates discussed above :) 🙏 |
3 participants
A branch that was worked on primarily during the summer months.
In cleanup processes now..
test_lattice_stats_Note: As we rely on regex for the parser, it is "breakable", depending on the type of input files structures it receives. However, it is important to note that any of the impactX examples inside of the dashboard is completely supported by the parser.