Package conflicts exist between proto and non-proto modules.

[woop]

Java classes that are currently generated from Protobuf definitions have overlapping package names with Feast modules. This causes conflicts when trying to import classes from Feast modules.

For example the CoreService.proto uses feast.core, which is also in use by the Feast Core module: example.

Proposal is to prefix all proto generated packages with .proto

[Yanson]

I'm not 100% convinced that proto really explains well what the classes are. How about something like api or model?

[woop]

I'm not 100% convinced that proto really explains well what the classes are. How about something like api or model?

api is being used for interfaces like storage-api, and model could be used for our domain models, so I am pretty sure we wouldn't want to use that.

The problem is that protos have both services and types. So if we have to use those terms then I would still nest them under proto., so maybe something like .proto.api and proto.models.?

[Yanson]

I don't see a problem using api for both proto specs and Java APIs, they shouldn't conflict.

• feast.core.api.CoreServiceProto
• feast.storage.api.writer.FeatureSink

The differentiation here could be one is an external / client API and one is internal. Adding that information to the package could be valuable.

Unless there is a second implementation, the tech doesn't need to be in the package name.
E.g. if you had something like feast.core.api.proto and feast.core.api.http or feast.core.api.rest or feast.core.api.graphql, I could understand.

So I might suggest something like client and internal. I'm not sure of the value in splitting out the models / dtos from the services in proto. They are all part of the API definition.

[Yanson]

BTW, I'm not going to lose sleep over this so don't let me block you.

[woop]

Always appreciate the time taken. I'm going to remove the 0.5 project (should have been a milestone anyway).

We can optimize for flagging the protos by including it in their package path, which is what I was going for earlier. I like the idea of knowing when I am dealing with protos and not obscuring that. At the same time the proto classes do contain the name proto in them, so it should be quite evident, so I dont feel to strongly about this.

Your proposal doesn't look bad either. if sharing the .api path doesn't conflict then that also seems reasonable. My main concern here is just conflicts.

Would be good to hear your thought on this @ches, especially if you have an existing standard for your Scala SDK.

[ches]

Would be good to hear your thought on this @ches, especially if you have an existing standard for your Scala SDK.

We do use com.agoda.feast.proto as base prefix to Feast's current packages for protos, e.g. com.agoda.feast.proto.core. ScalaPB has a nice feature to easily do this adjustment.

(This should be public Real Soon Now, since v0.3.7 jars got published I just have a little bit of tidying up to do, and some sign-offs. v0.4 included although it's not tested in practice yet).

I'm not personally in favor of api package for protos, at least for me that signals programming interface in the public Java interface storage-api sense rather than remote service API sense (although I'm sure it would be understood that way too, but it would muddle intentions IMO). I don't consider storage-api internal, I believe it's intended for Feast adopters to be able to extend, potentially pluggably from outside Feast's build in the future.

To me protos are wire format that user code should never extend, the opposite of API.

I like the idea of knowing when I am dealing with protos and not obscuring that.

I mentioned elsewhere that this was intentional for me too. I want it to stand out for me because if I have to write it in an import statement of an SDK documentation example, I basically consider it a bug to fix.

Working directly with the proto objects is usually not a very pleasant API, and it takes control of compatibility away from a library author—codegen can change, protobuf-java runtime can change, you may intentionally change protos in ways where source and semantic compatibility could be preserved by wrappers/proper models.

It's extra work to write and a bit of overhead, but worth it in the long run IMO.

I'm sympathetic to "abstraction over implementation" argument for naming though. I'd be happy to call a package perhaps wire instead of proto.

I'd also share that I heard a talk from the Python ecosystem awhile back where the speaker stressed something like,

Packages are for preventing name collisions, not defining a taxonomy.

and I really liked it. The Java ecosystem has package mania and half the time I'm wishing I could access a package private thing in a test but the project has 10 classes and 10 packages…

Anyway, that's even further off on a tangential rant than I had veered already.

[woop]

Thanks for the detail @ches. You've swayed my opinion at least. I'd like to take the proto route. I think wire does abstract, but it also obscures. I like the idea of spotting these proto weeds growing in the garden.

@Yanson I will submit a PR unless you want to hit the brakes here. This doesn't have to land in 0.5.

[Yanson]

I'm not so opinionated about this, so you guys go ahead. I prefer proto to wire though, as it is more meaningful.