Fix issue 292 adhoc services and passing session through #293

trjaffe · 2022-01-26T19:40:10Z

The original issue is simple to fix in scs.py. But in order to test it, I needed to be able to call a development server that didn't have the right certificate information. So I wanted to be able to do this:

session=requests.Session()
session.verify=False
conetest=vo.dal.SCSService('https://dev.server/vo/cone?table=foobar',session=session)
links=results[0].getdatalink(session=session)

etc. So I added the option to specify the session in the .getdatalink() method. It in turn calls some DAL methods that already take an optional session, so the change to help me debug was small and will be useful in future. It's undoubtedly also useful in other methods, but there might be a better way to do this. For instance, in DALResults, the method from_result_url() (called by getdatalink()) calls use_session() from utils/http.py, which creates a new session by default. Instead of creating a new one, DALResults could use self._session. In that case, the above call to getdatalink() wouldn't have to specify the session and would automatically inherit the one used in the cone service, the one already defined with verif=False.

Thoughts?

… functions for debugging.

codecov · 2022-01-26T19:44:14Z

Codecov Report

Merging #293 (1d2281e) into main (b5b33e8) will decrease coverage by 0.01%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##             main     #293      +/-   ##
==========================================
- Coverage   75.50%   75.49%   -0.02%     
==========================================
  Files          44       44              
  Lines        5128     5125       -3     
==========================================
- Hits         3872     3869       -3     
  Misses       1256     1256

Impacted Files	Coverage Δ
pyvo/dal/tap.py	`73.37% <50.00%> (ø)`
pyvo/dal/adhoc.py	`66.58% <83.33%> (+0.48%)`	⬆️
pyvo/__init__.py	`100.00% <100.00%> (ø)`
pyvo/dal/scs.py	`68.57% <100.00%> (ø)`
pyvo/dal/ssa.py	`56.00% <100.00%> (ø)`
pyvo/utils/compat.py	`100.00% <100.00%> (ø)`
pyvo/_astropy_init.py

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

msdemlei

I've never looked into the inner workings of passing around the session, but these changes look very reasonable.

On changing the MRO on SCSResults: Let's go for it.

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

Ape17

This is done by extracting non-core terms -- really, anything with a URI that starts with something that is not the core URI -- and excepting it from our normalisation.

Add missing code and data files back to dist wheels.

andamian · 2022-03-17T16:26:27Z

There are a number of simple workarounds (kludges) to this problem:
1.

import os
from requests.packages.urllib3.exceptions import InsecureRequestWarning
os.environ['CURL_CA_BUNDLE'] = ''   # disable cert check
requests.packages.urllib3.disable_warnings(InsecureRequestWarning)  # ignore warning

The CURL_CA_BUNDLE can be set in the environment without touching the library code.
2. Add session.verify = False to

pyvo/pyvo/utils/http.py

Line 29 in 9d5c501

return session

However, the ability to pass an external session might have its own merits (apart from setting verify=False) and should probably be considered in the context of the upcoming proposed changes to SSO (https://wiki.ivoa.net/twiki/bin/view/IVOA/SSO_next).

trjaffe · 2022-05-05T16:56:22Z

What do I need to do to get this through?

pyvo/dal/adhoc.py

… functions for debugging.

trjaffe · 2022-05-05T18:52:46Z

Apparently my rebase screwed everything up. Might be best to nuke it all from orbit and start over.

bsipocz · 2022-05-05T19:05:27Z

Apparently my rebase screwed everything up. Might be best to nuke it all from orbit and start over.

I'm happy to fix it for this PR if you prefer. (you might not have pushed the rebased version back with --force? )

trjaffe · 2022-05-05T20:39:15Z

Apparently my rebase screwed everything up. Might be best to nuke it all from orbit and start over.

I'm happy to fix it for this PR if you prefer. (you might not have pushed the rebased version back with --force? )

I did. Isn't that the correct way? But why does the PR now show that I changed 31 files when I only changed a couple of them? I was wondering if the fact that I forked before master became main would cause git any confusion. If it's not fixable, I can close this, delete my fork and start fresh and do a new PR.

bsipocz · 2022-05-05T21:08:09Z

Yes, it's the correct way.
I'm just guessing around for the source of the issue, I've seen it many times, but it's still puzzling how people end up in it.
Maybe you didn't fetch the astropy version before you did the rebase?

Anyway, at this point, an interactive rebase is needed on the freshly fetched astropy version. And it has to be interactive to be able to remove the duplicated commits.
(https://github.com/bsipocz/pyvo/tree/rebased_issue292)

andamian · 2022-05-05T22:48:47Z

Apparently my rebase screwed everything up. Might be best to nuke it all from orbit and start over.

That's exactly why I prefer the "Squash and merge" approach. I've screwed it up myself so many times that I think that the benefits of rebasing are not worth it.

bsipocz · 2022-05-05T23:19:48Z

Apparently my rebase screwed everything up. Might be best to nuke it all from orbit and start over.

That's exactly why I prefer the "Squash and merge" approach. I've screwed it up myself so many times that I think that the benefits of rebasing are not worth it.

Haha, I firmly believe that there are usually one or two tricky steps, and if one identifies them they will be set up to be a rebase wizard for life. Typical issues are that the rebase branch is not the up-to-date one but either the default branch from the fork, or an outdated version from upstream. And other typical issue is that force push isn't used instead the offer by github to update is accepted (and that lands a 'main' merge commit in the branch). Even in the case when it doesn't introduce extra unrelated commits, it brings in a nonlinear branch structure that is not nice to work with if/when a bisect or any history digging is required.

IMO the squash on merge has a ton of negative side effects, 1: it doesn't create a merge commit on the main branch but puts the squashed commit on it directly. That messes up a lot of the release scripts (you may not use those in pyvo, but they are super useful to identify any changelog and backport inconsistencies before they end up in a release) and possibly with the backports, too.
2) And while "squash and merge" is fine for small PRs like this one, in the case of a big feature I would prefer to have multiple commits that carry the history of the development, e.g. easier to go back and check when and possibly why a bug got introduced. It's not that easy to bisect and land on a thousand-lines worth of change instead of a logical chunk of a few dozens or a hundred.

Anyway, I'm not a maintainer here, so I'm OK with whichever workflow you decide to follow.

(one rather good resource to practice some of the scenarios is https://learngitbranching.js.org/, especially the remote and advanced topics)

andamian · 2022-05-06T17:29:47Z

I agree 100% (and thanks for the resource). Re-basing makes sense for large projects or commits but the effort is not justified in all cases. That's why in PyVO, oftentimes we've been accepting "Squash and merge".

trjaffe · 2022-05-11T14:44:57Z

Closing in favor of cleaner PR #327

Adrian Damian and others added 6 commits January 17, 2022 14:40

Remove astropy-helpers submodule

b830403

First attempt

193b635

First second attempt

c3ac5d7

First attempt (3)

4a5c9b7

Typo

e42e658

Fix for issue 292 adhoc services; passing session through to datalink…

fc9eb7d

… functions for debugging.

trjaffe marked this pull request as draft January 26, 2022 19:42

msdemlei approved these changes Jan 28, 2022

View reviewed changes

Adrian Damian and others added 17 commits February 11, 2022 17:59

Rework after code review

6c394ab

Rework after code review

ab8d3a7

Adding session to SSAService init.

ac91598

Fixed typo

1b813b4

More rework after code review

dc4c248

Updated to use packaging.version

4825922

Update pyproject.toml

646c9be

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

Update setup.cfg

781f88b

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

Merge pull request #290 from andamian/ape17

b837808

Ape17

Handle custom terms in datalink.bysemantics again. (#299)

365e1ae

This is done by extracting non-core terms -- really, anything with a URI that starts with something that is not the core URI -- and excepting it from our normalisation.

DOC: Fix coverage badge [ci skip] (#301)

42f4842

Preparing release v1.2.2

00da970

Add missing code and data files back to dist wheels.

62d4b5d

Merge pull request #303 from tomdonaldson/fix_release_build

be7d043

Add missing code and data files back to dist wheels.

Preparing release v1.3

8de00de

Back to development: v1.4.dev

a824d7d

Fix whitepace in CHANGES.rst

9d5c501

Adrian and others added 2 commits March 17, 2022 13:48

Fixed typo (#311)

7a0b7da

update requirements (#308)

b39eef7

trjaffe marked this pull request as ready for review May 5, 2022 15:49

zoghbi-a mentioned this pull request May 5, 2022

Fixes datalinks in scs (#320) by changing order of parent classes in dal.scs.SCSResults #321

Closed

zoghbi-a reviewed May 5, 2022

View reviewed changes

pyvo/dal/adhoc.py Outdated Show resolved Hide resolved

bsipocz added this to the v1.4 milestone May 5, 2022

trjaffe added 3 commits May 5, 2022 14:32

Fix for issue 292 adhoc services; passing session through to datalink…

79a7f76

… functions for debugging.

Merge branch 'issue292' of https://github.com/trjaffe/pyvo into HEAD

4cd2d87

Simpler session reuse

06cd75e

And fixing iterator again after last change

1d2281e

trjaffe mentioned this pull request May 11, 2022

Adding session to SSAService and passing it through DatalinkService. #327

Merged

trjaffe closed this May 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue 292 adhoc services and passing session through #293

Fix issue 292 adhoc services and passing session through #293

trjaffe commented Jan 26, 2022

codecov bot commented Jan 26, 2022 •

edited

Loading

msdemlei left a comment

andamian commented Mar 17, 2022 •

edited

Loading

trjaffe commented May 5, 2022

trjaffe commented May 5, 2022

bsipocz commented May 5, 2022 •

edited

Loading

trjaffe commented May 5, 2022

bsipocz commented May 5, 2022

andamian commented May 5, 2022

bsipocz commented May 5, 2022

andamian commented May 6, 2022

trjaffe commented May 11, 2022

Fix issue 292 adhoc services and passing session through #293

Fix issue 292 adhoc services and passing session through #293

Conversation

trjaffe commented Jan 26, 2022

codecov bot commented Jan 26, 2022 • edited Loading

Codecov Report

msdemlei left a comment

Choose a reason for hiding this comment

andamian commented Mar 17, 2022 • edited Loading

trjaffe commented May 5, 2022

trjaffe commented May 5, 2022

bsipocz commented May 5, 2022 • edited Loading

trjaffe commented May 5, 2022

bsipocz commented May 5, 2022

andamian commented May 5, 2022

bsipocz commented May 5, 2022

andamian commented May 6, 2022

trjaffe commented May 11, 2022

codecov bot commented Jan 26, 2022 •

edited

Loading

andamian commented Mar 17, 2022 •

edited

Loading

bsipocz commented May 5, 2022 •

edited

Loading