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

Add LBMS commissioning proposal support #145

Merged
merged 7 commits into from
Feb 14, 2025

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