[MNT] Removed session_file_path from aggregate results

[rmanaem]

Closes #210

Changes proposed in this pull request:

• Updated create_query function to not query session_file_path

Checklist

• PR has an interpretable title with a prefix ([ENH], [FIX], [REF], [TST], [CI], [MNT], [INF], [MODEL], [DOC]) (see https://neurobagel.org/contributing/pull_requests for more info)
• PR links to GitHub issue with mention Closes #XXXX
• Tests pass
• Checks pass

For new features:

• Tests have been added

For bug fixes:

• There is at least one test that would fail under the original bug conditions.

[coveralls]

Pull Request Test Coverage Report for Build 6789540380

• 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 99.826%

---

Totals
Change from base Build 6739448284:	0.0%
Covered Lines:	575
Relevant Lines:	576

---

💛 - Coveralls

[alyssadai]

Thanks for the PR @rmanaem. I have a few implementation questions:

• A quick test of a local aggregate-mode API with your changes shows that the subject_data field still appears in the response, just as an empty list []. Should we remove this key entirely from the aggregate response to avoid API user confusion? Or maybe populate it with an informative value like "protected"?
• I noticed that this logic is still kicking around - not sure if we need it anymore
  

  api/app/api/crud.py

  Lines 140 to 144 in 47d3b7e

  	if util.RETURN_AGG.val:
  	subject_data = list(
  	{"session_file_path": file_path}
  	for file_path in group["session_file_path"].dropna()
  	)

• I see you reverted your initial change to the test_data aggregate response fixture, but I am in favour of updating it to reflect what the actual aggregate response looks like - wdyt?

Happy to have a chat about these decisions if that'd be helpful.

[rmanaem]

A quick test of a local aggregate-mode API with your changes shows that the subject_data field still appears in the response, just as an empty list []. Should we remove this key entirely from the aggregate response to avoid API user confusion? Or maybe populate it with an informative value like "protected"?

I like the protected idea but to just to be sure is this subject_data: ["protected"] what you had in mind?

I noticed that this logic is still kicking around - not sure if we need it anymore

Great catch, I'll get rid of that.

I see you reverted your initial change to the test_data aggregate response fixture, but I am in favour of updating it to reflect what the actual aggregate response looks like - wdyt?

I don't have a preference but If we were to do that I'd say both subjects in the test_data should have their subject_data updated instead of just one like my commit did. wdyt?

[alyssadai]

I like the protected idea but to just to be sure is this subject_data: ["protected"] what you had in mind?

I think so, although you probably have a better sense of what would work best with the query tool. Since the API currently always expects a list of dicts for subject_data,

api/app/api/models.py

Line 63 in 47d3b7e

subject_data: list[dict]

I'm even wondering if we should just return subject_data: "protected" and make the model accept either a dict or a string value for this key. Alternatively, as said above, we could make this key entirely optional in the model.

I don't have a preference but If we were to do that I'd say both subjects in the test_data should have their subject_data updated instead of just one like my commit did. wdyt?

Yes, since the fixture represents a single response I think all the subjects should be updated!

[rmanaem]

I think so, although you probably have a better sense of what would work best with the query tool. Since the API currently always expects a list of dicts for subject_data,

Yea that's why I prefer not removing the subject_data field. The query tool checks for that so I'd rather have it in the response even if it's empty.

I'm even wondering if we should just return subject_data: "protected" and make the model accept either a dict or a string value for this key. Alternatively, as said above, we could make this key entirely optional in the model.

I had completely forgotten about that. I like the idea of extending the model to accept string values as well.

[alyssadai]

I think this looks good! 🧑‍🍳