Skip to content

Conversation

@stuartcampbell
Copy link
Collaborator

@stuartcampbell stuartcampbell commented Feb 11, 2025

This PR adds in support to deal with LBMS facilities commissioning proposals.

There are a number of places that this code can be improved by removing hard coded values for the PASS IDs for the respective commissioning proposal types (I've created an issue #146 )

Testing on my local dev instance...

http://127.0.0.1:8000/v1/proposals/commissioning?facility=lbms

{
  "count": 0,
  "commissioning_proposals": [],
  "beamline": null,
  "facility": "lbms"
}

http://127.0.0.1:8000/v1/proposals/commissioning?facility=nsls2

{
  "count": 30,
  "commissioning_proposals": [
   "311955",
    "312922",
    "311272",
    ...
    <truncated list>
    ...
    "312051",
    "313772"
  ],
  "beamline": null,
  "facility": "nsls2"

}

http://127.0.0.1:8000/v1/proposals/commissioning?facility=cfn

{
  "count": 0,
  "commissioning_proposals": [],
  "beamline": null,
  "facility": "cfn"
}

And with no facility parameter... http://127.0.0.1:8000/v1/proposals/commissioning

{
  "count": 34,
  "commissioning_proposals": [
    "311955",
    "312922",
    "311272",
    ...
    <truncated list>
    ...
    "317763",
    "317764"
  ],
  "beamline": null,
  "facility": null
}

And specifying a beamline http://127.0.0.1:8000/v1/proposals/commissioning?beamline=PDF

{
  "count": 1,
  "commissioning_proposals": [
    "312977"
  ],
  "beamline": "PDF",
  "facility": null
}

Which still gives the same answer if we select the wrong facility for the beam line
http://127.0.0.1:8000/v1/proposals/commissioning?beamline=PDF&facility=lbms

{
  "count": 1,
  "commissioning_proposals": [
    "312977"
  ],
  "beamline": "PDF",
  "facility": null
}

I think that this will also fix #140

@stuartcampbell stuartcampbell self-assigned this Feb 11, 2025
@stuartcampbell stuartcampbell marked this pull request as ready for review February 11, 2025 22:54
Copy link
Contributor

@JunAishima JunAishima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking at this and proposal_service.is_commissioning(), it seems like the pass_type_id field is actually crucial for handling commissioning proposals - we should make this a mandatory field (is currently Optional).

looks comprehensive in getting the functions we need, otherwise!

Copy link
Contributor

@JunAishima JunAishima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pass_service also needs to be updated to use the appropriate commissioning proposal type

diff --git a/src/nsls2api/services/pass_service.py b/src/nsls2api/services/pass_service.py
index 747659a..fccbf5c 100644
--- a/src/nsls2api/services/pass_service.py
+++ b/src/nsls2api/services/pass_service.py
@@ -149,9 +149,8 @@ async def get_commissioning_proposals_by_year(
         error_message: str = f"Facility {facility_name} does not have a PASS ID."
         logger.error(error_message)
         raise PassException(error_message)
-
-    # The PASS ID for commissioning proposals is 300005
-    url = f"{base_url}/Proposal/GetProposalsByType/{api_key}/{pass_facility}/{year}/300005/NULL"
+    pass_commissioning_type = await get_commissioning_proposal_type(facility_name)
+    url = f"{base_url}/Proposal/GetProposalsByType/{api_key}/{pass_facility}/{year}/{pass_commissioning_type.pass_id}/NULL"
 
     try:
         pass_commissioning_proposals = await _call_pass_webservice(url)

@danielballan
Copy link
Collaborator

@JunAishima Can you open a PR against this PR branch with that change?

@JunAishima
Copy link
Contributor

JunAishima commented Feb 12, 2025

@JunAishima Can you open a PR against this PR branch with that change?

Yes, that will work well - I am looking for any further issues I can handle in the same PR

stuartcampbell#2 (Note, Stu's github site)

@nmaytan
Copy link
Collaborator

nmaytan commented Feb 13, 2025

I've tested this using a local instance of nsls2api against production PASS (due to issues with passdev) and, after including Jun's changes, I think this is working.

In [35]: r = c.get(url="http://localhost:8080/v1/proposals/commissioning?facility=lbms")

In [36]: r.content
Out[36]: b'{"count":4,"commissioning_proposals":["317761","317762","317763","317764"],"beamline":null,"facility":"lbms"}'

In [37]: r = c.get(url="http://localhost:8080/v1/proposals/commissioning?facility=nsls2")

In [38]: r.content
Out[38]: b'{"count":23,"commissioning_proposals":["317830","318602","315950","316912","316025","318008","317851","317919","317926","317839","317807","316889","317793","317866","316014","317752","315993","317888","316005","316882","317799","316878","316771"],"beamline":null,"facility":"nsls2"}'

With this we can sidestep for now the question of recording facilities in the proposal data model. Though as we discussed, there is more thinking needed on that point (for example, this won't fix facility filtering for the newer proposals endpoint)

@JunAishima JunAishima self-requested a review February 14, 2025 16:06
@JunAishima
Copy link
Contributor

looking at this and proposal_service.is_commissioning(), it seems like the pass_type_id field is actually crucial for handling commissioning proposals - we should make this a mandatory field (is currently Optional).

I will remove this request for the current PR and have made an issue - #149

Approving PR.

Copy link
Contributor

@JunAishima JunAishima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve - also requires stuartcampbell#2 to fully handle LBMS proposals correctly.

@danielballan danielballan merged commit 1d4a8a7 into NSLS2:main Feb 14, 2025
4 checks passed
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

Successfully merging this pull request may close these issues.

Endpoint to retrieve commissioning proposals needs facility query parameter

4 participants