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

Make JCB in Job Status and Control interface an abstract type #208

Closed
dongahn opened this issue May 29, 2015 · 9 comments
Closed

Make JCB in Job Status and Control interface an abstract type #208

dongahn opened this issue May 29, 2015 · 9 comments

Comments

@dongahn
Copy link
Member

dongahn commented May 29, 2015

As discussed in PR #205

@trws
Copy link
Member

trws commented Jul 16, 2015

Just a quick note on this. At least for my purposes, it would be fine for it to remain JSON so long as it ceases to expose the json_c type itself. Preferably handing out const handles to a JSON string, or similar.

@dongahn
Copy link
Member Author

dongahn commented Jul 16, 2015

Thanks. Also a quick note. One of the reasons that I didn't want to use JSON strings as input/output was that this seemed to add high overheads. E.g., the JSC user will construct a JSON-C JCB blob and covert it to a JSON string to pass it to JSC, then JSC will reconstruct it to an JSON-C object to send to the KVS. A question. If I hide JSON-C under an abstract type and provide accessor functions as suggested by @garlick, does this work better or worse with Python binding?

@trws
Copy link
Member

trws commented Jul 16, 2015

It would probably be slightly better in that I wouldn't have to convert it to a string, then interpret the string as json to convert it into a python data structure then go back through it the whole way on the other end. I'm not sure I entirely see the overhead point though. The KVS uses JSON strings internally, so once we have an interface for just directly passing them in I think much of the overhead you're considering would go away. Perhaps I'm not thinking of the specific problem case though.

@dongahn
Copy link
Member Author

dongahn commented Jul 16, 2015

Thanks. I will think about this more when I revisit this issue and may pick your brain a bit at that time.

Perhaps more importantly, how urgent is it to wrap the python binding to this more completely for your HT scheduling milestone -- don't want to drag your feet and will adjust my priority accordingly.

@trws
Copy link
Member

trws commented Jul 16, 2015

Not that important. I made a wrapper class for handling json_object unpacking and conversion that makes it relatively straightforward to handle things like this. I more brought up the issue because we've been making a concerted effort, mainly @garlick really has, to excise library types from the interface, such as zmsg_t and json_object.

It's more to allow the clients, in this case the python bindings, to use a native JSON library rather than having to deal with creating a json_c-specific object. Right now I do it like this:

j = Jobj(
json.dumps(python_object)
)

Which converts the python object into a JSON string with a native library then has json_c interpret it and create a json_object to pass. This is really, really inefficient. Using any other json library would entail a similar issue as long as it's in the API.

@garlick
Copy link
Member

garlick commented Jul 16, 2015

Right: kvs_put (flux_t h, const char *key, const char *json_str); or something similar is coming soon.

@chu11
Copy link
Member

chu11 commented Apr 4, 2017

Is this issue resolved now? Handled via 92169c5?

@garlick
Copy link
Member

garlick commented Apr 4, 2017

It does look like the original issue, as qualified by @trws, was solved (by 00ac7e6 and then 4eddc63 I think).

I'm not sure if @dongahn had planned to go further with this, but I think maybe we could close this one and reopen another for any remaining issue.

@trws
Copy link
Member

trws commented Apr 4, 2017

Yeah, I'm good with that as solved. Thanks for the triage.

@trws trws closed this as completed Apr 4, 2017
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

4 participants