Refactor photometry handler #2648

mcoughlin · 2022-02-08T23:47:20Z

This PR refactors the photometry handler to enable posting from a different API (useful for the ATLAS and PS1 PRs).

mcoughlin · 2022-02-09T00:03:48Z

@profjsb It looks crazy pants, but this is just removing the 4 space indent for most of the functions at the start of the photometry handler and calling them directly.

Fix test: test_sources.py::test_sources_include_detection_stats.

profjsb

This looks great but I'm still worried about the LOCKs. Looking at https://stackoverflow.com/a/47256217 it seems to make sense for us to put those LOCK lines inside a try statement then in the except make sure we do a DBSession.rollback() to make sure we release the lock.

profjsb · 2022-02-09T17:30:54Z

skyportal/handlers/api/photometry.py

+    if "altdata" in data and not data["altdata"]:
+        del data["altdata"]
+    if "altdata" in data:


I dont see how line 184 ever evaluates to True given the lines above.

a = []
assert not a

So checks for empty list?

mcoughlin · 2022-02-09T17:44:55Z

@profjsb So basically

DBSession().execute(
    f'LOCK TABLE {Photometry.__tablename__} IN SHARE ROW EXCLUSIVE MODE'
)
try:
    ids, upload_id = insert_new_photometry_data(
        df, instrument_cache, group_ids, stream_ids, user
    )
except ValidationError as e:
    return log(f"Unable to post photometry: {e}")

=>
try:
DBSession().execute(
f'LOCK TABLE {Photometry.tablename} IN SHARE ROW EXCLUSIVE MODE'
)
ids, upload_id = insert_new_photometry_data(
df, instrument_cache, group_ids, stream_ids, user
)
except Exception as e:
DBSession.rollback()
return log(f"Unable to post photometry: {e}")

stefanv · 2022-02-09T19:49:17Z

At minimum, the lock would need to be wrapped in a try: ... finally: ... clause. But if the app gets brought down, the lock could end up hanging around.

I believe we have duplication checks on the DB level, so another option is to just do the check, try the insert, and catch the error if it fails?

mcoughlin · 2022-02-09T20:08:04Z

@stefanv Can you help me understand how your second option is different from above?

stefanv · 2022-02-10T18:43:03Z

@mcoughlin I looked at your snippet again and I think the except catch-all is fine; sorry for any confusion. Just use either DBSession() or DBSession, but not both.

As an aside, we are now able to use the new SQLAlchemy syntax:

with DBSession() as session: ...

…nto photometry_handler

mcoughlin · 2022-02-10T21:33:50Z

@stefanv can you see if you are happier with this? I put your try except and with DBSession() as session around the LOCK bits.

stefanv

Thanks @mcoughlin! Looks good. There's just that one docstring left hanging, and a question re: multiple types of session objects.

stefanv · 2022-02-10T21:38:53Z

skyportal/handlers/api/photometry.py

+
+def add_external_photometry(json, user):
+    """
+    ---


This docstring seems to be misplaced

what does that mean?

Ah I see, because now we just return the log()

Well, it looks like a handler (GET, POST, PUT, etc.) docstring in YAML, whereas I would expect a standard Python function docstring that describes the two arguments: json and user.

Ah yes, hopefully more clear now?

stefanv · 2022-02-10T21:39:56Z

skyportal/handlers/api/photometry.py

@@ -43,6 +45,10 @@
 _, cfg = load_env()


+log = make_log('api/photometry')
+Session = scoped_session(sessionmaker(bind=DBSession.session_factory.kw["bind"]))


Still used? Do we need both DBSession and Session?

I have usually seen us using DBSession() in the APIs and Session when called external to the APIs (like when we do the IOSync stuff).

…nto photometry_handler

stefanv · 2022-02-10T22:19:12Z

skyportal/handlers/api/photometry.py

            )
+        except Exception as e:
+            session.rollback()
+            return log(f"Unable to post photometry: {e}")


Log does not return anything

skyportal/handlers/api/photometry.py

Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com>

mcoughlin · 2022-02-11T14:55:28Z

@profjsb please merge if comfortable with this now.

profjsb

Looks great. We should make another issue to look into why we get 400 and 500 sometimes (not suppose to happen).

profjsb · 2022-02-11T15:04:21Z

skyportal/tests/api/test_photometry.py

@@ -2258,7 +2263,7 @@ def test_problematic_photometry_1276(
        data=payload,
        token=super_admin_token,
    )
-    assert status in [401, 500]
+    assert status in [400, 500]


Why is 400 getting returned anywhere? It's worth understanding this

HTTP 500 is internal server error. BAD.

This is something that should be investigated by scouring those logs.

mcoughlin added 2 commits February 8, 2022 17:46

Refactor photometry handler

6bf748b

rebase with latest updates

ab8e797

mcoughlin requested a review from profjsb February 9, 2022 00:32

Testing source detection_stats on Github's CI (skyportal#2644)

a5c1a39

Fix test: test_sources.py::test_sources_include_detection_stats.

mcoughlin mentioned this pull request Feb 9, 2022

ATLAS forced photometry #2498

Merged

Merge branch 'skyportal:master' into photometry_handler

3077c94

mcoughlin requested a review from stefanv February 9, 2022 14:37

profjsb reviewed Feb 9, 2022

View reviewed changes

mcoughlin force-pushed the master branch from 10ab385 to ab6e2a5 Compare February 10, 2022 18:39

mcoughlin added 5 commits February 10, 2022 12:55

Try stefan's syntax

10ffe09

Merge branch 'photometry_handler' of github.com:mcoughlin/skyportal i…

0788c35

…nto photometry_handler

Make put encompass whole PUT

f1c3299

updated error code because now in lock try except

6a6044a

Merge branch 'skyportal:master' into photometry_handler

b2c28b6

mcoughlin mentioned this pull request Feb 10, 2022

WIP: Issues using Candidate and Photometry Handlers #2647

Closed

stefanv reviewed Feb 10, 2022

View reviewed changes

mcoughlin added 2 commits February 10, 2022 15:59

Improve docs

9a783a1

Merge branch 'photometry_handler' of github.com:mcoughlin/skyportal i…

62a5196

…nto photometry_handler

stefanv reviewed Feb 10, 2022

View reviewed changes

Remove the return comment...

9763d18

stefanv reviewed Feb 10, 2022

View reviewed changes

skyportal/handlers/api/photometry.py Outdated Show resolved Hide resolved

stefanv approved these changes Feb 10, 2022

View reviewed changes

skyportal/handlers/api/photometry.py Outdated Show resolved Hide resolved

mcoughlin and others added 2 commits February 10, 2022 16:24

Update skyportal/handlers/api/photometry.py

80ef8fe

Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com>

Update skyportal/handlers/api/photometry.py

a3bdfa4

Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com>

mcoughlin requested a review from profjsb February 11, 2022 00:26

profjsb approved these changes Feb 11, 2022

View reviewed changes

profjsb merged commit c927b44 into skyportal:master Feb 11, 2022

mcoughlin deleted the photometry_handler branch August 30, 2022 14:18

Theodlz mentioned this pull request Feb 12, 2024

Photometry handler: pandas warning #4765

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor photometry handler #2648

Refactor photometry handler #2648

mcoughlin commented Feb 8, 2022

mcoughlin commented Feb 9, 2022

profjsb left a comment

profjsb Feb 9, 2022

mcoughlin Feb 9, 2022

mcoughlin commented Feb 9, 2022

stefanv commented Feb 9, 2022

mcoughlin commented Feb 9, 2022

stefanv commented Feb 10, 2022 •

edited

Loading

mcoughlin commented Feb 10, 2022

stefanv left a comment

stefanv Feb 10, 2022

mcoughlin Feb 10, 2022

mcoughlin Feb 10, 2022

stefanv Feb 10, 2022

mcoughlin Feb 10, 2022

stefanv Feb 10, 2022

mcoughlin Feb 10, 2022

stefanv Feb 10, 2022

mcoughlin commented Feb 11, 2022

profjsb left a comment

profjsb Feb 11, 2022

stefanv Feb 11, 2022

stefanv Feb 11, 2022

Refactor photometry handler #2648

Refactor photometry handler #2648

Conversation

mcoughlin commented Feb 8, 2022

mcoughlin commented Feb 9, 2022

profjsb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcoughlin commented Feb 9, 2022

stefanv commented Feb 9, 2022

mcoughlin commented Feb 9, 2022

stefanv commented Feb 10, 2022 • edited Loading

mcoughlin commented Feb 10, 2022

stefanv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcoughlin commented Feb 11, 2022

profjsb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanv commented Feb 10, 2022 •

edited

Loading