Remove dependency search #39742
Merged
Remove dependency search #39742
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 480ecee...1a8bc57.
|
eseliger
reviewed
Aug 1, 2022
| @@ -654,13 +654,6 @@ | |||
| "group": "Code intelligence", | |||
| "default": false | |||
| }, | |||
| "codeIntelLockfileIndexing.enabled": { | |||
There was a problem hiding this comment.
can we remove this or do we have to deprecate it? every config that has this key will no longer validate and thus cannot be updated anymore.
There was a problem hiding this comment.
Yeah, I considered that, but I highly doubt someone has ever turned this on except for me. It wasn't mentioned in any blog post or changelog entry. But yeah, can deprecate it instead.
There was a problem hiding this comment.
Done. But I removed the experimentalFeatures.DependenciesSearch flag. I think experimentalFeatures flags should be removable.
eseliger
reviewed
Aug 1, 2022
eseliger
reviewed
Aug 1, 2022
eseliger
reviewed
Aug 1, 2022
migrations/frontend/1659351276_remove_dependency_search/down.sql
Outdated
Show resolved
Hide resolved
eseliger
approved these changes
Aug 1, 2022
mrnugget
force-pushed
the
mrn/remove-dep-search
branch
from
10f2c0f
to
0561d22
Compare
Short version: we currently don't have the time and resources to properly invest in dependency enough to make it both a feature that's useful for customers and one that meets our technical bar. We use version control. There's no advantage to keeping code around "in case we need it again" (we can always restore using `git`), but multiple disadvantages (people stumble on this code and wonder whether/how it's used, it needs to be maintained, its tests take up runtime, ...). So that's why we're removing it. Longer version: As [@sqs announced](https://sourcegraph.slack.com/archives/C03L2R35ENL/p1659142618246179), dep search is one of the "things we are saying NO to as we look to 4.0". It might come because, because we're saying No "(until it can be done solidly, which is not now)". I myself wrote on Slack: > I’ve had multiple conversations with @sqs about this in the past week > and we came to the conclusion that in order to make dependency search a > useful feature we’re proud of, we’d need to invest a lot more time and > resources. But now is not a good time to do that. We have other problems > to solve right now that are higher-priority than adding a new feature > we’re unsure about. > Even if I were to continue working on Dependency Search for the next, > say, 6 months, it’s unclear whether Dependency Search in its current > “architecture” (in the product and in the technical sense) is what would > solve some important customer problems. We would maybe need to rethink > it from the ground up. For example: focus on reverse-dependency search > from the beginning, or focus more on “search dependency relationships” > instead of “search inside dependencies”. Or, in the words of @eric > Fritz: “the lego bricks don’t seem to line up”. So, here we go, let's remove the code :) Technicalities: * It drops all of the database tables used by dependency search * It removes all of the code used for dependency search: backend, frontend, ... * It removes docs * It removes the feature/site-config flag * It does **NOT** remove the code in `./internal/codeintel/dependencies` that's used for packages.
mrnugget
force-pushed
the
mrn/remove-dep-search
branch
from
0561d22
to
12b4eee
Compare
|
Why are you deleting so much of the dependency indexing stuff? Can't we leave some of the background stuff in place? I'd like to use what we were building for code intel as well... |
See PR description:
The dependency indexing stuff was built for dependency search. It's then completely unused. When would you use it? And can't you restore then? |
Yes, that's actually fine. |
efritz
approved these changes
Aug 1, 2022
|
(Give me a sec, I need to remove more mentions of |
stefanhengl
approved these changes
Aug 1, 2022
|
I was thinking of using the dependency primitive in search (not the aspirational "full on dependency search for realsies" feature we were planning) for some exploratory things, but I guess I'll branch and restore if it amounts to anything 😆. My impression is that Post edit: my questions are not directed at anything 4.0 related or talking about this as a feature release. Just want to see any high level points on what the limitations are to using this in the current state, say, internally. |
Well, kinda. There's no obvious bugs or problems with it. It's not being removed because it falls over 4 times a day. But: with the old index-repo-on-request model of dependency search it worked out of the box, but was unreliable (no comprehensive results, different degrees of quality changing from language to language and lockfile version to version, lot of timeouts, huge load on the backend in spurts, ...). The new model was reliable and explicit, but its out-of-the-box experience was not great (you had to setup a package host connection, then lockfile-index the relevant repositories, possibly the packages that then result from this indexing, then query) and didn't ultimately solve problems customers were interested in (everyone is interested in full dependency graph that's trustworthy, searching for something inside a set of dependencies is not something folks ask for). Or in other words: you had to do the same dance you have to do to get precise code intel, except that it wasn't 100% precise and absolutely-not precise for most languages we supported, and the feature wasn't as useful as precise code intel. If you're curious, you can watch the Zoom recording mentioned here to see what the thinking was ~2 weeks ago that lead to this.
I'd say we've have to improve the out-of-the-box flow immensely, we'd have to invest in packages (see #packages for recent conversations around that), we'd need to improve the parsers for all languages we support (or reduce number of languages we support), then work hard to ensure we are comprehensive and don't return false positive or false negatives, etc. All of this combined makes it really uncomfortable to demo the feature to customers when knowing that we can't invest more than one IC working on it for forseeable future. |
|
Thanks @mrnugget that's exactly what I wanted to know. Makes sense given the cost of precise resolution vs easy-but-unreliable tradeoffs. I'll explore my ideas in a branch, maybe it can fold into future effort 😄 |
mrnugget
added a commit
that referenced
this pull request
Aug 16, 2022
This should've been part of #39742 but I missed it. @lguychard also removed something else I missed: https://github.com/sourcegraph/sourcegraph/pull/40164/files This here now removes the highlighting/completion of the `repo:deps|revdeps|dependencies|dependents` predicates. I kept the metrics in as to not break anything. This should fix #40160.
mrnugget
added a commit
that referenced
this pull request
Aug 16, 2022
This should've been part of #39742 but I missed it. @lguychard also removed something else I missed: https://github.com/sourcegraph/sourcegraph/pull/40164/files This here now removes the highlighting/completion of the `repo:deps|revdeps|dependencies|dependents` predicates. I kept the metrics in as to not break anything. This should fix #40160.
jdorfman
pushed a commit
that referenced
this pull request
Aug 17, 2022
This should've been part of #39742 but I missed it. @lguychard also removed something else I missed: https://github.com/sourcegraph/sourcegraph/pull/40164/files This here now removes the highlighting/completion of the `repo:deps|revdeps|dependencies|dependents` predicates. I kept the metrics in as to not break anything. This should fix #40160.
mrnugget
added a commit
that referenced
this pull request
Aug 19, 2022
This was deleted along with dependency search in #39742.
mrnugget
added a commit
that referenced
this pull request
Aug 19, 2022
This was deleted along with dependency search in #39742.
mrnugget
added a commit
that referenced
this pull request
Aug 24, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Short version
We currently don't have the time and resources to properly invest in dependency enough to make it both a feature that's useful for customers and one that meets our technical bar.
We use version control. There's no advantage to keeping code around "in case we need it again" (we can always restore using
git), but multiple disadvantages (people stumble on this code and wonder whether/how it's used, it needs to be maintained, its tests take up runtime, ...).So that's why we're removing it.
Longer version
As @sqs announced, dep search is one of the "things we are saying NO to as we look to 4.0".
It might come because, because we're saying No "(until it can be done solidly, which is not now)".
I myself wrote on Slack:
So, here we go, let's remove the code :)
Technicalities
It drops all of the database tables used by dependency searchIt keeps the tables around until the next release, to be backwards-compatible.frontend, ...
./internal/codeintel/dependenciesthat's used for packages.
Test plan
main-dry-runbuild for this branch