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

Let's remove generated C code from the repo? #1032

Open
ekalosak opened this issue Sep 23, 2023 · 3 comments
Open

Let's remove generated C code from the repo? #1032

ekalosak opened this issue Sep 23, 2023 · 3 comments
Labels

Comments

@ekalosak
Copy link
Contributor

From the initial discussion in PR #1020 (merged), this thread is intended to be a home for discussion and implementation planning around the effort to respect newer guidance for generated C (cython/cython#5089).

IMHO the forward compatibility concerns are valid, and are not theoretical for GPy (see PR 1020). So now we need to make sure removing the generated C doesn't dramatically impact user installation. That is, if we take out the C code, do we hit any of the following issues?

  • pip install GPy becomes unbearably slow on a standard workstation
  • pip install GPy has new unseen build dependencies

What am I missing? Do we need to update tests to account for this? What's the shortest path to "done"?

@ekalosak
Copy link
Contributor Author

#720
Looks like tests depend on the C files, currently.

Which to me means #1030 blocks this initiative.

@MartinBubel
Copy link
Contributor

As I have not yet digged sufficiently deep into the entire cython & generated c code stuff, I have a rather practical proposal that might give us a little bit more insight: Lets check out how the pip install GPy times are actually looking like when we remove the generated C code? Maybe its not as bad as expected?

However, its your last point thats more tricky and might be difficult to test before releasing it to the public.

@ekalosak
Copy link
Contributor Author

We can start with a branch in PR marked [DRAFT], that will make it available early to beta testers ASAP.

Then we probably will have to update the CI. Come to think of it... nose upgrade doesn't block this. I think we instead only need to update the pre-test build to include generating the C on the CI host. But who knows, that will become clear upon investigating the CI.

I'm not planning too jump on this right now, but I am happy to chat.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants