Arvados

Radhika Chippada wrote:

Documentation updates

• "Retrieve a list of items which are associated with the given group by ownership (i.e., the group owns the item)". => Can it be simplified as “Retrieve a list of items owned by the group”

• “N.B.” => We use “Note:” in most other places and I think is easier to understand.

Both done.

• In the Arguments table, the vertical line separator between type and description columns is disappearing. Seems like due to the parentheses characters in limit default 100

Hmm, I can't reproduce this in Firefox 35. See the attached screenshot. Even with different window sizes, I couldn't get it to disappear. The markup appears correct, both in Textile and generated HTML. What browser are you using? Do you see the same issue for the table under the list method? (I copied most of the text from that, but didn't modify it.)

• “Because adding access tokens to manifests can be computationally expensive, the manifest_text field is not included” => May be we can simplify it by saying “Because adding manifests can be computationally expensive, the manifest_text field is not included”

This text is copied verbatim from the collections list page. I'm a little wary about changing it because I don't want users to get the impression that it's difficult for Arvados to send blocks of plain text back and forth.

Code updates:

• map { |s| "#{ar_table_name}.#{ActiveRecord::Base.connection.quote_column_name s}" }

was there a bug here in earlier version? Just curious.

Sort of. We need this change for the method to work right when GroupsController is querying Collections with a @select parameter. The old code would always use "groups" as the table name here, and crash looking for columns in the collections table. This change ensures we're referring to the table name for whatever's in @objects.

• New tests in arvados_model_test.rb

Can you add “job” in the test case names since all these are referring to job data model?

The goal is to test the method in ArvadosModel. I can't call that directly, because ArvadosModel is effectively abstract, so I had to pick some concrete implementation. I picked Job, but there should be nothing Job-specific about the tests—it should be possible to write equivalent tests for any subclass of ArvadosModel, as long as it has attributes that meet the criteria described.

If there's some way to help make clearer that picking Job is effectively an implementation detail, I'm happy to do that. One way I tried to emphasize that is by putting these in arvados_model_test.rb rather than job_test.rb. I'm concerned that putting "job" in the test title will give the opposite impression, that we're testing something specific about the Job class, when that's not the point.

• It would be nice to add a performance test for visiting the Data collections tab for this project (browsing_test.rb). However, since we do not yet have a jenkins job running to measure performance against qr1hi, this will not help compare as of now.

I had the same thought about a performance test, but yeah, it feels like we don't have good infrastructure for testing performance handling of large collections yet. Besides, performance is only part of the motivation for the branch—the other half is to make this method more consistent with the collections list method, which should happen anyway.

Now at 7c5a88d. Thanks.

The project named in this bug now loads very quickly, so I'm marking this as closed. Admittedly there's been some fallout from this API change, but we're tracking those separately.