added pythonnet check with warning output #235
Conversation
There was a problem hiding this comment.
Looking good. @klmcadams please add some unit tests for everifying the correct behavior of this implementation.
Also, the documentation warnings that rise are also appearing in the main branch when the workflow is ran. Seems to me like some kind of new side-effect... See https://github.com/pyansys/pymechanical/actions/runs/4879631252/jobs/8706395546
I tested it locally and it seems like the root cause is the new Python dev docs. They must have done some change to the way they define booleans for Python 3.12.X...
Easy fix would be to change in conf.py line 63 as follows:
"python": ("https://docs.python.org/3", None),
Could you also take care of that?
|
@klmcadams please implement the docs fix in a different PR - just to keep them separated - and then update your branch. Thanks for your time and efforts! |
…core/embedding/app.py Co-authored-by: Roberto Pastor Muela <[email protected]>
…ding/app.py Co-authored-by: Roberto Pastor Muela <[email protected]>
I made a branch called fix/docwarning for the doc pipeline issue. No problem! Let me know if I can do anything else. |
|
@klmcadams nice work! I left a few comments/suggestions but I'll review in more detail tomorrow. Could you please update the changelog? |
Thank you! I will upload the unit tests as soon as possible. |
Thank you! That sounds good. I can update the changelog as well. |
|
Hi @koubaa, @MaxJPRey & @RobPasMue, I added the unit tests for the pythonnet changes and added a line to the changelog. I looked into importlib.metadata as Mohamed suggested, and think it works better than the pip freeze implementation I did earlier. Instead of parsing through the pip freeze output, it now calls the distribution('pythonnet'). If the pythonnet is in the environment, it creates the warning message, otherwise it prints a message saying pythonnet was not found. I added apt install -y python3.8-venv to the ci_cd.yml file because when I was getting venv errors for the embedded/launcher tests, the output said to run "apt install -y python3.8-venv" no matter what version it was. After checking in my most recent commit and the automatic PR was running, the Launch Testing and Coverage (3.10) step was the only one that seemed to get stuck in unit testing. Whenever I canceled that step and ran it again, it passed. I'm not sure how often this happens, but we could deploy the workflow again before merging into main just to be sure if you would like. I believe those are all the major updates with this PR. Let me know if you have any questions or concerns. Thanks! |
There was a problem hiding this comment.
Tests are looking good! Nicely done @klmcadams. I left some suggestions/minor improvements. I'll leave it up to @koubaa to approve and merge, but for me, this PR is looking good.
Co-authored-by: Roberto Pastor Muela <[email protected]>
…ding/initializer.py Co-authored-by: Roberto Pastor Muela <[email protected]>
Co-authored-by: Roberto Pastor Muela <[email protected]>
Co-authored-by: Roberto Pastor Muela <[email protected]>
Co-authored-by: Roberto Pastor Muela <[email protected]>
Co-authored-by: Roberto Pastor Muela <[email protected]>
Co-authored-by: Roberto Pastor Muela <[email protected]>
Co-authored-by: Roberto Pastor Muela <[email protected]>
Co-authored-by: Roberto Pastor Muela <[email protected]>
Thank you so much! It was an interesting issue to work on. |
|
@klmcadams if you're willing to take any more I could suggest some |
Sure, feel free to message me on teams or assign one to me. I can look this weekend when I have more free time |
If pythonnet is installed and a remote instance is used, there should not be a warning. Pymechanical embedding should work, but some of the APIs will not work if pythonnet is installed. Since the remote instance should still work whether or not pythonnet is installed, it was determined a warning would be a better use, so users are aware of the conflict, but it doesn't stop it from running.