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

go: jsonformat package is not go gettable #18

Closed
sam-virta opened this issue Jul 16, 2020 · 20 comments
Closed

go: jsonformat package is not go gettable #18

sam-virta opened this issue Jul 16, 2020 · 20 comments

Comments

@sam-virta
Copy link

sam-virta commented Jul 16, 2020

Currently the imports in the jsonformat go package require consumers to also be using bazel. It would be nice if this implementation was directly consumable (as in, via go modules)

If you aren't planning on supporting this, please let me as we will try a go mod hack or begrudgingly maintain a fork 🙂

@nickgeorge
Copy link
Collaborator

Great idea. We have a maven jar for Java, and will have pypi support for Python, so it makes sense to also support go modules. We'll investigate making this coexist with bazel - as long as that plays nicely we should be able to get this out soon.

@andyblum
Copy link

andyblum commented Jul 30, 2020

I want to emphasize what @sam-virta stated. I am trying to use your code to convert FHIR JSON to proto. It is very frustrating as every road leads through Bazel. My use case is that I would like to be able to run an AWS Lambda in any language that will convert FHIR JSON data to proto. So I learned GO just for the purpose of the conversion as I wanted to take advantage of your code. WWYD? Please advise and thank you for reading. Am I better off using the advice [here](https://stackoverflow.com/questions/38406211/how-to-convert-from-json-to-protobuf to accomplish) to accomplish my goal of converting JSON FHIR to Proto?

@nikklassen
Copy link
Member

This is a high priority for us given the feedback from this issue

@suyashkumar
Copy link
Contributor

suyashkumar commented Aug 14, 2020

Quick update on this: I've made some internal changes in v0.5.4 that update our import paths (to the familiar github.com prefixed go-gettable paths) and code location, the first step in making the library go-gettable and go mod compliant.

The next step will be to generate and include the go proto code in the repo and have the go libraries depend directly on them (instead of relying on the bazel-based proto code generation). :)

@loafoe
Copy link

loafoe commented Aug 25, 2020

@suyashkumar running into the following:

main.go

package main

import (
  _ "github.com/google/fhir/go/jsonformat"
)

func main() {
}
> go mod tidy
go: finding module for package github.com/google/fhir/go/jsonformat
go: downloading github.com/google/fhir v0.5.4
fhir imports
	github.com/google/fhir/go/jsonformat: create zip: module source tree too large (max size is 524288000 bytes)

Should we wait for .6 for Bazel-free usage?

Related Go ticket: golang/go#40780

@suyashkumar
Copy link
Contributor

suyashkumar commented Aug 25, 2020

@loafoe We won't be able to fully support go modules until we generate and include the go proto code in the repo (as mentioned above, this is the next step).

However, the repo size limitation you run into is interesting, thank you for raising this! I'm not super familiar with why our repo size is so large (@nickgeorge may know more): looking at the fhir-0.5.4.zip: https://github.com/google/fhir/archive/v0.5.4.zip (it seems like its around 111MB compressed, and ~900MB uncompressed). This may be a good reason to consider separating this library out from the rest of the FHIR repo if we will not be able to get around this limit, which will also be a blocker for adopting go modules. We'll discuss some more internally on next steps and go from there. Thank you!

@nickgeorge
Copy link
Collaborator

nickgeorge commented Aug 25, 2020

hmm is the go module size just the size of the whole repo? If it's just the go code, I'd be pretty surprised if it crossed some threshold.

As for the repo itself - my guess is most of that is the versions of the FHIR spec itself we have checked in, for use in proto generation. I have a plan to run the proto generation off of MPM location as published by FHIR, at which point I'd likely expunge the checked-in versions of the spec from the git history. That should result in a pretty significant drop in overall size.

@suyashkumar
Copy link
Contributor

suyashkumar commented Aug 25, 2020

Thanks @nickgeorge! Makes sense, it seems like the checked in spec is indeed ~700MB. Because go modules (and go get) rely on cloning the root repository to fetch and build the dependency, that's probably why it's running into an issue.

One thing to note: while go modules may have this issue, go get does not seem to mind w/ go1.14.3 (however, as mentioned we need to check in the generated go protos before this will fully work without bazel).

@wesdean
Copy link

wesdean commented Oct 15, 2020

Is there any way to generate the proto files ourselves and include them in a Go project so we can use the jsonformat package? If there is, are there instructions on how to do that?

@suyashkumar
Copy link
Contributor

Hi @wesdean! I wouldn't say this is recommended, but to answer your question: you can always use the protocol buffer compiler protoc with the Go plugin to produce the generated Go proto code if you vendor this repository as a dependency in your repo. There would then be some work to reconcile the proto import paths in the jsonformat package with however you tell protoc to output the Go proto code as well.

Keep in mind that if you use Bazel as a build system for your project, you should not have to worry about any of this (Bazel will handle building the jsonformat package and building the required proto dependencies, though there are some setup costs with Bazel).

In terms of where we are with go mod / go get support--there are various internal mechanics that need to be put into play to support the vendored go proto code reliably alongside internal tooling, which the team is working on putting in place for this issue among juggling other priorities. There is also likely another refactor needed to reduce the repo size to be workable with Go modules, which is part 2 of what needs to be done to have true Go modules support as discussed by Nick above.

@suyashkumar
Copy link
Contributor

suyashkumar commented Dec 3, 2020

Hi folks, with the release of v0.7.0, we have go get and go modules support for the Go libraries 🎉.

You can check out the updated FHIR examples that now also use Go modules, and you can start using github.com/google/fhir/go as a Go module right away.

Let us know if you have any questions.

@nikklassen
Copy link
Member

Feel free to reopen if you encounter any issues

@wesdean
Copy link

wesdean commented Dec 8, 2020

I am getting this error. Any ideas on how to resolve?
go mod vendor
go: finding module for package github.com/google/fhir/go
github.com/anthem-ai/hos-adaptor-rest/internal/server imports
github.com/google/fhir/go: module github.com/google/fhir/go@latest found (v0.0.0-20201203001644-a2580b6ea022), but does not contain package github.com/google/fhir/go

@suyashkumar
Copy link
Contributor

Thanks for mentioning this! Can you give us a little more context on your setup? In particular your Go version, and if you're setting any environment variables (e.g. GOPROXY)?

The easiest way to debug for us would be to try to set up a minimal repository that can reproduce this error and start from there. A minimal working example with go modules can be seen here (clone into your GOPATH, and you should be able to go build any of the Go binaries).

Out of curiosity, does the minimal working example above work with your configuration (e.g. GOPROXY, etc?)

@nickgeorge nickgeorge reopened this Dec 8, 2020
@suyashkumar
Copy link
Contributor

suyashkumar commented Dec 9, 2020

@wesdean I've also added a minimal working example (that doesn't require Synthea) here, and have also set it up with GitHub actions so you can have a small reproducible working playground where go modules currently work if that's helpful.

@wesdean
Copy link

wesdean commented Dec 9, 2020

Thanks @suyashkumar. I solved it. I was trying to import
"github.com/google/fhir/go"
instead of
"github.com/google/fhir/go/jsonformat"

@suyashkumar
Copy link
Contributor

Great to hear! Let us know if you run into any other issues. Thanks!

@wesdean
Copy link

wesdean commented Dec 9, 2020

Sorry to be a bother, but I'm having some trouble when serializing a Bundle. I'm getting an error on the Birth Date field of a Patient record.
marshalMessageToMap repeated field entry: marshalRepeatedFieldValue entry[0]: marshalMessageToMap optional field resource: marshalMessageToMap optional field birth_date: serialize date: invalid timezone offset: America/Los_Angeles
The Birth Date object looks like this.
value_us:19700515 timezone:"America/Los_Angeles" precision:DAY

When I set the Birth Date to nil, I get other serialization errors such as
marshalMessageToMap repeated field entry: marshalRepeatedFieldValue entry[3]: marshalMessageToMap optional field resource: marshalMessageToMap optional field subject: invalid reference type *core.Reference
This error comes from an Encounter.

I get these errors when my service starts up.
hos-adaptor-rest | 2020/12/09 16:02:15 WARNING: proto: message google.fhir.r4.core.Bundle.Entry is already registered
hos-adaptor-rest | previously from: "github.com/anthem-ai/hos-proto/gen/go/google/fhir/r4/core"
hos-adaptor-rest | currently from: "github.com/google/fhir/go/proto/google/fhir/proto/r4/core/resources/bundle_and_contained_resource_go_proto"
hos-adaptor-rest | A future release will panic on registration conflicts. See:
hos-adaptor-rest | https://developers.google.com/protocol-buffers/docs/reference/go/faq#namespace-conflict

Could my issue have something to do with the bundle being from the hos-proto types?
I don't have the issue when not using this type. Our plan is to remove these proto types from our code but I would like to know if this is likely the issue so I don't keep plugging at it fruitlessly.

Thanks

@nikklassen
Copy link
Member

marshalMessageToMap repeated field entry: marshalRepeatedFieldValue entry[0]: marshalMessageToMap optional field resource: marshalMessageToMap optional field birth_date: serialize date: invalid timezone offset: America/Los_Angeles
The Birth Date object looks like this.
value_us:19700515 timezone:"America/Los_Angeles" precision:DAY

FHIR proto timezones need to specified as an offset, i.e. "-08:00"

marshalMessageToMap repeated field entry: marshalRepeatedFieldValue entry[3]: marshalMessageToMap optional field resource: marshalMessageToMap optional field subject: invalid reference type *core.Reference
Could my issue have something to do with the bundle being from the hos-proto types?

We only support the Google FHIR protos in with this library. Some protos may happen to work by nature of reflection similarities, but this marshaller is specifically for our our protos.

@nikklassen
Copy link
Member

nikklassen commented Dec 9, 2020

@wesdean Please open another issue if you have further questions and we'll be happy to help you there

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

No branches or pull requests

7 participants