Skip to content

104 migrate fink broker to dataservice#112

Open
phycodurus wants to merge 20 commits intodevfrom
104-migrate-fink-broker-to-dataservice
Open

104 migrate fink broker to dataservice#112
phycodurus wants to merge 20 commits intodevfrom
104-migrate-fink-broker-to-dataservice

Conversation

@phycodurus
Copy link
Member

@phycodurus phycodurus commented Feb 17, 2026

Adds FinkDataService class; deprecates FinkBroker

My tests are passing with poetry run python tom_fink/tests/run_tests.py and a local, but un altered tom_base (dev).

In this case, make the Fink broker appear in TOMToolkit's list
of DataServices, when tom_fink is INSTALLED.

TOMToolkit will look for certain methods in the AppConfig of a
TOM's INSTALLED_APPS to ask the App how to integrate it into the TOM.
This makes apps more self contained and reduces app-specific
configuration data in settings.py.
This is part of the "DataServices refactor" of the Fink broker
interface.
@phycodurus phycodurus linked an issue Feb 17, 2026 that may be closed by this pull request
7 tasks
- minimal form changes: remove GenericQueryForm.common_layout
  (it is replaced by layout in the BaseQueryForm)
- the FinkBroker has been removed
  - FinkBroker.to_generic_alert and FinkBroker.to_target functionality
    condensed into create_target_from_query
  - FinkerBroker.fetch_alerts is now FinkDataService._fetch_alerts
    and FinkDataServic.query_service just calls _fetch_alerts.
- minimal form changes: remove GenericQueryForm.common_layout
  (it is replaced by layout in the BaseQueryForm)
- the FinkBroker has been removed
  - FinkBroker.to_generic_alert and FinkBroker.to_target functionality
    condensed into create_target_from_query
  - FinkerBroker.fetch_alerts is now FinkDataService._fetch_alerts
    and FinkDataServic.query_service just calls _fetch_alerts.
condense query results in to structure suitable for presenting
in the selectable "create target" table.
The target table row dictionary was updated to be appropriate for
one-target-one-row. These changes follow from that and use the
appropriate dict keys to get the data to create the target.
@phycodurus phycodurus requested a review from jchate6 February 23, 2026 21:26
@phycodurus phycodurus marked this pull request as ready for review February 23, 2026 21:26
@jchate6 jchate6 moved this to Needs Review in TOM Toolkit Feb 27, 2026
Setting up these DataService subclass properties in this way
allows the base class to display this info on the RunQueryView,
etc.
...using the DataService base class methods and
compatibly-written partials.
...by implementing query_photometry() method
The data is already in memory (in the response JSON payload).
So, there's not advantage to converting it to an Iterator.
Other refactor: group target method together and photometry
methods togeter.
@phycodurus
Copy link
Member Author

The latest commits implement Photometry updating and divide the Form into Simple and Advanced using the DataSerivce base class-provided mechanisms.

Copy link
Contributor

@jchate6 jchate6 left a comment

Choose a reason for hiding this comment

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

Also, the README needs to be updated.

Let me know if you want to go through this, or have me make these changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably actually have some tests.


class FinkDataService(DataService):
"""
This is an Example Data Service with the minimum required
Copy link
Contributor

Choose a reason for hiding this comment

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

need real doc string


def build_query_parameters(self, parameters, **kwargs):
"""
Use this function to convert the form results into the query parameters understood
Copy link
Contributor

Choose a reason for hiding this comment

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

Better doc string.

self.query_parameters = parameters
return self.query_parameters

def _fetch_alerts(self, parameters: dict) -> Iterator:
Copy link
Contributor

Choose a reason for hiding this comment

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

This function effectively ignores the point of having a build_query_parameters. You are including both the functionality of translating form fields into fink-readable parameters (class_name, n_alert = parameters["classsearch"].split(",")) and the submission of a valid request to fink.
This makes this more difficult to test and maintain.

"stopdate": end,
},
)
elif len(parameters["ssosearch"].strip()) > 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

If this functionality isn't working, it should be (temporarily) removed from the query form and this method.

:param self: Description
:param target_result: Dict of Target data for selected Target
:type target_result: Dict[str, Any]
:param kwargs: Description
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove

"""

# extract values from query target_result and create Target
# NOTE: use constructor, not get_or_create, CreateTargetFromQueryView will save the Target
Copy link
Contributor

@jchate6 jchate6 Mar 12, 2026

Choose a reason for hiding this comment

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

This is now in to_target to keep it out of the view and give the data service access.

# search among the target's aliases for something that starts with 'ZTF'
ztf_aliases = target.aliases.filter(name__startswith='ZTF').order_by('-created')
if not ztf_aliases.exists():
raise ValueError(
Copy link
Contributor

Choose a reason for hiding this comment

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

this should use QueryServiceError imported from tom_dataservices.dataservices
I'll add a note about this to the basic docs.

# Photometry
#

def query_photometry(self, query_parameters, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

No Docstring

Copy link
Contributor

Choose a reason for hiding this comment

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

A lot of your comments in this section would be better captured in the doc string.

target=target,
timestamp=timestamp,
data_type=data_type,
source_name='Fink',
Copy link
Contributor

Choose a reason for hiding this comment

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

Use self.name for consistency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Needs Review

Development

Successfully merging this pull request may close these issues.

Migrate Fink Broker to DataService

2 participants