-
Notifications
You must be signed in to change notification settings - Fork 22
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
PoC for ragna-base
package with just base dependencies
#405
Conversation
I don't think we can avoid that. Quoting from the
Note that there is no mention of Lines 8 to 9 in e397acb
which we need to have |
I wonder if you're suggesting for a monorepo architecture, that is absolutely possible. So I guess we'll need to have two different directories i.e. |
My idea would be to have a
The install should be fairly straight-forward. For a local development environemnt we run
Could you elaborate what you want to test? Do you mean our test suite or something else? If you mean the test suite, why do you want to test both? We should just require
Sure. If we have a clean way to avoid all of the above, I prefer that as well. |
Yes, I was thinking that the test suite might break in case of |
Still, I'm not sure if this'd raise and import issue for optional dependency methods (if it'd we would need to add informative warnings and error and also test |
From these couple of days I've been trying to configure a concurrent |
Depending on a
Meaning, we could do it, but even the official maintainers are advising against it. The exception that they make is for building C extensions, but we are not doing that. For this PR this means: if we cannot do it with just a |
Oh thanks Philip for making it clear in the start. Fortunately I got one more solution while scrolling web today (thanks to this comment) which looks much promising to me as compared to |
I don't understand that. Basically, the only duplication would be the metadata in the Note that the new |
…while symlinking with any of them dynamically
With the current changes I'm able to build
And
|
We can also get rid of |
I see. Sorry, with the comment above I misunderstood that we'd had to have |
Hi, @arjxn-py. I had another idea about how to go about doing this, so I am posting this comment as requested by @pmeier. I would say that having the changes in Even though the The dynamic dependencies currently noted in import os
from setuptools import setup
with open('requirements.txt') as f:
ragna_dependencies = f.read().splitlines()
with open('requirements-base.txt') as f:
base_dependencies = f.read().splitlines()
name = 'ragna-base' if os.environ.get("BUILD_RAGNA_BASE") else 'ragna'
dependencies = base_dependencies if os.environ.get("BUILD_RAGNA_BASE") else ragna_dependencies
setup(
# ...
name=name,
version='',
packages=['ragna'], # or find_packages() with include= and exclude= attributes
install_requires=dependencies,
# and so on
) while the rest of the project metadata stays in This is where you can switch between particular requirements at the time of running the build frontend ( P.S. Sorry, I meant to post this yesterday – I didn't hit the "Comment" button and this comment stayed as a draft, oops! Now that I think of it, to avoid using P.P.S. Another option is to look at how |
Hey @agriyakhetarpal, thanks for such a descriptive comment, much helpful. I'd be happy to integrate the suggested changes and would potentially get rid of nox.
Just to let you know, I've tried to figure out what they've been doing and not been able to get more information about the significant of environment variable they're using and how they're handling it. |
For current changes Build for Ragna package & Build for ragna-base package both builds for ragna in spite of me trying to overwrite the name and dependencies in shim In next commit I'll compare it to the approach using |
Hi @pmeier, this past week I had been doing some hit & trials without And this current solution appear to work seamlessly which works without the need to have two separate The couple of processes that are failing should be easily fixable but before that I'm marking this ready for review for an initial review. Thanks. cc: Also thanks @agriyakhetarpal because of which I started to look for solution other than the For reference, here is the build for |
Co-authored-by: Agriya Khetarpal <[email protected]>
ragna-base
package with just base dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @arjxn-py for the current version. I left a few comments, but nothing major.
However, one thing is not how we discussed it. The difference between ragna
and ragna-base
in this PR is that ragna
has a few more dependencies than ragna-base
. But the ragna
package still distributes our code itself rather than depending on ragna-base
.
ragna
should be a meta package that does depend on ragna-base
(same version number) and all optional dependencies, but should not distribute any code itself.
Co-authored-by: Philip Meier <[email protected]>
Yes, current Note that a metapackage does not have any API of its own; it merely provides dependencies for other packages. Therefore, if we want to make ragna a metapackage, it should install ragna-base and optional dependencies as needed. Which we may try once the |
Incorporate suggestions from Code Review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, current
ragna
is not a metapackage, but I feel that the current method is cleaner and fulfills the requirements of a metapackage.
It does not. The problem is that you now have two separate distributions (ragna
and ragna-base
) that install the ragna
package into your environment. Worse, they don't even know of each other.
So what happens if someone specifies something like pip install ragna==0.3.0 ragna-base==0.2.0
? No one would do this in their direct requirements, but maybe ragna
and ragna-base
are being pulled in as dependencies?
To avoid this, we need to have the ragna
distribution a metapackage that installs ragna-base
. That way, the install command will net you dependency conflict. And there is no ambiguity which distribution actually supplies the ragna
package.
Note that a metapackage does not have any API of its own; it merely provides dependencies for other packages. Therefore, if we want to make ragna a metapackage, it should install ragna-base and optional dependencies as needed. Which we may try once the
ragna-base
is published on pypi maybe (i.e. inrequirements.txt
we should be able to replace all base dependencies with justragna-base
)
Yes, this is exactly what I want to achieve. Why do you want to wait for ragna-base
being published though?
Thanks for your review & apologies for bit of a delay in response.
I tried doing that locally with wheels of
I observed that these two were getting installed subsequently without error & while I try to do
I also then uninstalled
I then needed to re-install
As far as I observed, priority is |
Assuming that you mean this (So sorry if I assumed wrong) :
I'd like to make a note that both of these serves the same purpose but there's a potential problem with if we were to use ragna-base from pypi as a dependency for ragna. They would not be in sync, because even if we updated the dependencies for ragna-base and then wanted to in a corresponding manner update the dependencies for ragna, we would need to put out a release for ragna-base on PyPI first and then publish ragna after that, which could cause developers troubles.
It's because package manager wouldn't be able to identify what |
I am really sorry if i'm being a little persuasive, I just want to make sure that we explore optimized solutions plus all the edge cases that I could think of at the moment which may cause potential bottleneck situations in the future. However I'm trying to answer to the best of my current knowledge & capacity I believe that I may also not be fully correct so I'd be happy to move forward upon how you decide after a collective discussion. |
Just a gentle follow up, I've replaced the base dependencies with Sorry, i didn't want to bother you with pinging and was expecting your response whenever you had chance. |
Don't worry, you have every right to ping when I don't get back to you. I'm sorry for that. I should have a chance to look at this in the next two days. |
This PR aims to give user the total control with
pip install ragna
i.e. which will correspond to currentragna[all]
& also proposes a proof of concept forragna-base
which will give user the fine-grained control i.e. will further correspond to currentragna
.Related to #397
NOTE