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

setup.py fix for M1 installation #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nleroy917
Copy link

Here's what I have locally if you wanted to take a look. To help you out with any potential debugging I just made a PR so you can see it all inline.

@marvinquiet
Copy link
Owner

Hi Nathan @nleroy917,

Thank you so much for your contribution.

However, I might need to first test if it still works well for MacOS or Linux. Then I will get back and to merge the pull request.

Thank you so much for your help and efforts! Please do not hesitate to let me know if you encounter any issues with running Cellcano.

Sincerely,
Wenjing

@nleroy917
Copy link
Author

No problem! I just wanted to share what I got working for me relative to the entire codebase. If you don't think its necessary to merge, that's ok!

@lujiaying
Copy link
Collaborator

Hi Nathan and Wenjing,

Thank you for your efforts to make Cellcano setup more robust.
I propose an alternative way to dynamically handle the setup requirement, by looking into current platform and machine types.

Best,
Jiaying

@nleroy917
Copy link
Author

Relevant stack overflow per @lujiaying's suggestion.

However, the issue seems resolved by just removing any pinned dependencies - this fixes the issue while still being agnostic to platform. My personal opinion would be that it should be up to the user with the M1 to specifically install tensorflow-macos if they wish to use metal acceleration, but I'll defer to you all to make the decision there since its your package!

If I can help in any way let me know.

@lujiaying
Copy link
Collaborator

Gotcha. I agree "removing any pinned dependencies" is a good solution.

Another thing is we want to make numbers reported in the paper replicable, and sometimes certain packages may conduct aggressive upgrade (tensorflow 1.0 -> 2.0 makes a lot of API incompatible). It also impose challenges in terms of maintainess efforts if we need to keep every dependent third-party packages up-to-date.
I would suggest to keep and fix a pinned version, as it is a common practice for many open-source softwares. (e.g. https://github.com/autogluon/autogluon/blob/master/multimodal/setup.py)

@marvinquiet
Copy link
Owner

Hi Nathan @nleroy917,

Thank you so much for the wait and the willingness to contribute.

After discussing with Jiaying, we think it is better to keep pinned dependencies to make sure all the results are reproducible in the future. I am also worried that if removing those specific versions, people might not be able to run my software.

I would add a reference to this pull request and issue number in the documentation to let people know your solution. Thank you again for your detailed contribution and I really appreciate it!

Sincerely,
Wenjing

@lujiaying lujiaying linked an issue Jun 28, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't install Cellcano in M1 Mac
3 participants