-
Notifications
You must be signed in to change notification settings - Fork 51
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
Application always requires all gems #4316
Comments
Interesting -- a lot of those dependencies are regulated by global constants set in the initializer, as we need to control access to the features that depend on them in the UI. I wonder if it would be possible to move some of those to bundler groups, then load the groups based on the constants? |
@benwbrum I just edited my first comment to clarify that it's more likely that |
Let's talk a little more about open source distributions. Right now we have a development process that goes like this: Feature development happens in feature branches based off of Deployment happens in two different branches
Since there is code specific to the SaaS business that open source installations don't want, we have some commits which remove this from Thoughts? |
There is also the consideration of how huge of an impact does it make on the docker size and speed. We will also have to coordinate to open-source that we are doing this and they have to re-add the necessary gems back if they need it (which again as @benwbrum mentioned we still don't know fully which is which) But I agree that we do need to identify which is necessary for each cases (development, test, opensource, etc) |
Thanks for clarifying, @benwbrum! I would still like to differentiate between making FromThePage features optional (like OpenAI and Transkribus integrations, and SaaS-only aspects like redirecting the homepage to As @WillNigel23 says, coordinating with admins of other FromThePage instances is necessary, because it is likely that installation instructions may change. This issue should be considered together with #4291 as I think that no gems need to be removed from the Gemfile, just grouped (except uglifier, as it is not used). It is very likely that other parts of the code need to be changed, to be more selective in the gems that certain parts of the code require 'openai'
if ENABLE_OPENAI
OpenAI.configure do |config|
config.access_token = OPENAI_ACCESS_TOKEN
end
end Perhaps After #4291, I envision that installation instructions may include something like:
These examples are based on an idea to put the capistrano-related gems in a Perhaps some of the SaaS features/specifics could be extracted from the shared codebase into separate gems, eventually, so that building and deploying the SaaS instead of the basic version is a matter of configuration to include the extras instead of applying specific commits to disable and/or remove SaaS specifics. Or the views as they are should be updated to render SaaS specifics only if |
Good point on differentiating between product features and deployment-specific code. In an ideal world, So I think that it makes sense to conditionally load the OpenAI gem (and others) in Notifying other installations is also tricky -- we are friends with the folks running installations at University of Texas-Austin, University of Nebraska-Lincoln, and of course University of Leiden, and have corresponded briefly with folks at Utrecht and Melbourne, but we have no contact at all with the people running installations at Bolsano, Taipei, or Buenos Aires (and of course there are likely installations which we don't even know about.) Technically: I'd be very happy to merge the conditionalization of |
I tried to
bundle install
without the development and test groups of gems, but then the application would not start. I reported this in #4291 (comment)The OpenAI and Bento gems are bad examples, because the application itself relies on them. Capistrano and related gems are better examples, since they are used for deployment only (and I would not need them when I deploy the application in a Docker container).
The Bundler documentation on groups says:
1e8fa22 was the latest change to this line in application.rb; it's not clear from the commit message or in-line comments why this was necessary. I think it should be possible to change the call so that fewer gems are
require
d at runtime.The text was updated successfully, but these errors were encountered: