Skip to content

feat: Load CLR with Python.NET#421

Merged
LeeDongGeon1996 merged 4 commits intofacioquo:mainfrom
LeeDongGeon1996:fix/support-arm64
Feb 5, 2025
Merged

feat: Load CLR with Python.NET#421
LeeDongGeon1996 merged 4 commits intofacioquo:mainfrom
LeeDongGeon1996:fix/support-arm64

Conversation

@LeeDongGeon1996
Copy link
Collaborator

@LeeDongGeon1996 LeeDongGeon1996 commented Jan 30, 2025 •
edited
Loading

changes

@LeeDongGeon1996
Copy link
Collaborator Author

LeeDongGeon1996 commented Jan 30, 2025

both failed actions look like authorization issue.

@DaveSkender
Copy link
Member

DaveSkender commented Jan 30, 2025 •
edited
Loading

both failed actions look like authorization issue.

DaveSkender
DaveSkender reviewed Jan 30, 2025
View reviewed changes
Copy link
Member

@DaveSkender DaveSkender left a comment

Choose a reason for hiding this comment

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

Check unit tests as I’ve now added an ARM64 runner for checks that are currently failing. It may be, that this more ideal Python.NET find_dotnet_root() utility isn’t actually working yet, for this situation.

@LeeDongGeon1996
Copy link
Collaborator Author

LeeDongGeon1996 commented Jan 31, 2025 •
edited by DaveSkender
Loading

@DaveSkender Looks like the failed tests were false negatives for some reason. I re-run all tests and it all passed. Does it all passed before this PR?

yah, all green in my last couple PRs and on main push builds

@LeeDongGeon1996 LeeDongGeon1996 changed the title Fix: Support ARM64 feature: Load CLR with Python.NET Jan 31, 2025
@LeeDongGeon1996 LeeDongGeon1996 changed the title feature: Load CLR with Python.NET feat: Load CLR with Python.NET Jan 31, 2025
@facioquo facioquo deleted a comment from github-actions bot Jan 31, 2025
@LeeDongGeon1996
Copy link
Collaborator Author

LeeDongGeon1996 commented Jan 31, 2025

The root problem was my virtual environment on my local machine is on M1 Rosetta mode though. This proves the need that we should load clr through pythonnet. pythonnet is handling this case also.

@DaveSkender
Copy link
Member

DaveSkender commented Jan 31, 2025

The root problem was my virtual environment on my local machine is on M1 Rosetta mode though. This proves the need that we should load clr through pythonnet. pythonnet is handling this case also.

I'm wondering if the root problem from #408 was actually with venv? We might need a build test for that. I removing my own local venv instance a couple weeks ago due to problems.

DaveSkender
DaveSkender reviewed Jan 31, 2025
View reviewed changes
Copy link
Member

@DaveSkender DaveSkender left a comment

Choose a reason for hiding this comment

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

Just re-request a Review when you want me to look at this again. I'm not sure if you'd intended to or are interested in checking out these two open topics:

  1. Typings issue with clr .NET members, even if just dev env problem?
  2. Broken venv handling, or if that's even related to our code.

@LeeDongGeon1996
Copy link
Collaborator Author

LeeDongGeon1996 commented Feb 1, 2025 •
edited
Loading

I'm wondering if the root problem from #408 was actually with venv? We might need a build test for that. I removing my own local venv instance a couple weeks ago due to problems.

@DaveSkender Sorry, I made you confused.

The reason for #408:

  • This is because AddReference() does not work correctly on .NET9 over MacOS. You added DLL manually and now it works. I think finding DOTNET_ROOT manually is not a solution for here. That's why I removed that lines in this PR.

The reason for error on my virtual env:

So there was two different issues and venv is not a root cause of #408.

@LeeDongGeon1996
Copy link
Collaborator Author

LeeDongGeon1996 commented Feb 1, 2025 •
edited by DaveSkender
Loading

I don't think we need a bulid test for venv. This should be done on pythonnet their side. and it is already covered by them.

@LeeDongGeon1996
Copy link
Collaborator Author

LeeDongGeon1996 commented Feb 1, 2025

Typings issue with clr .NET members, even if just dev env problem?

@DaveSkender
I'm not sure but as I know that linting for .NET members are not supported since they are not python native things or even they don't provide the interface(like .pyi) that linter can recognize.

DaveSkender
DaveSkender reviewed Feb 2, 2025
View reviewed changes
Copy link
Member

@DaveSkender DaveSkender 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.

Makes sense, intuitively. One thing I'll want to check is whether macOS with just .NET 9 still works. The GitHub runners have .NET 8 and .NET 9 installed, so we many not be getting an entirely pure test on that scenario.

In my head, I'm still confused by some of this -- if we rollback the changes I made to fix a problem; how is it that the problem is still solved? Is it the Assembly.LoadFile() was the fix, and other stuff was unneeded -- I think that's what you're saying, which makes sense. I ran into multiple issues in debugging, so I may have fixed an intermediate problem that wasn't needed after I'd fixed something else.

DaveSkender
DaveSkender approved these changes Feb 2, 2025
View reviewed changes
Copy link
Member

@DaveSkender DaveSkender left a comment

Choose a reason for hiding this comment

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

I think I understand this better now, and was able to run in on a macOS with just .NET 9 installed.

@LeeDongGeon1996 LeeDongGeon1996 merged commit 3cf995a into facioquo:main Feb 5, 2025
15 checks passed
@LeeDongGeon1996 LeeDongGeon1996 deleted the fix/support-arm64 branch February 5, 2025 14:58
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 12, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@DaveSkender DaveSkender DaveSkender approved these changes

Assignees

No one assigned

Labels

None yet

Projects

Stock Indicators for Python
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Load CLR with Python.NET

2 participants

@LeeDongGeon1996 @DaveSkender