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

Added RACS and VAST Pilot surveys to Aladin #284

Merged
merged 5 commits into from
Sep 17, 2020
Merged

Conversation

ajstewart
Copy link
Contributor

Default loaded survey set to RACS.

There's just the CORS question, it may work fine in production?

Fixes #282.

@ajstewart ajstewart added enhancement New feature or request UI User Interface labels Aug 25, 2020
@github-actions github-actions bot added this to In progress in Pipeline Backlog Aug 25, 2020
@srggrs
Copy link
Contributor

srggrs commented Aug 25, 2020

I still have CORS issue. We need to fix that before merge.

@marxide
Copy link
Contributor

marxide commented Sep 2, 2020

This isn't a problem with our code or configuration. The ATNF web server isn't sending the appropriate CORS headers. However, you'll notice that loading the HiPS surveys from there still works!

Looking at the Aladin Lite source, it has a mechanism in place to circumvent CORS issues by using a proxy. To determine if it needs to proxy the request, it first attempts to fetch the HiPS properties file. If there is no error, it will then attempt to fetch the image tiles with CORS enabled. If there was an error (e.g. a CORS error), it will attempt to fetch the image tiles through a proxy. It is this error that gets logged to the console by the browser as it was a request that failed. Aladin has attached an error handler to the request to then proceed with proxied requests, but that won't stop the browser from logging an error for the initial failed request. As far as I know, there isn't anything we can do to prevent that.

To fix, I think we'd need to do any of the following:

  • Ask ATNF to configure the CORS headers on their web server (probably unlikely).
  • Ask the Aladin developers to change how they test for CORS support from remote servers (I don't have an alternative in mind).
  • Ask the Aladin developers to provide an option that we can explicitly set to always proxy requests for a given image survey.
  • Move the HiPS images to Nimbus which we control and set the CORS headers (we may not have the disk space).

Otherwise, I think we just ignore it and move on. We seem to only get one or two errors, and thankfully not one for each image survey.

@marxide
Copy link
Contributor

marxide commented Sep 2, 2020

For reference, this is where that initial "test" request happens in the Aladin Lite source

https://github.com/cds-astro/aladin-lite/blob/64fcc777dee6435e7d98534bb1d232259cb1e5c3/src/js/HpxImageSurvey.js#L143-L174

It would be nice if we could trigger the else block with an extra option as I mentioned above, but I don't think it's worth pursuing that option. Aladin Lite will log a lot of errors when you navigate to an area of the sky not covered by the HiPS survey, so we're likely going to have JS errors anyway.

@ajstewart
Copy link
Contributor Author

To fix, I think we'd need to do any of the following:

  • Ask ATNF to configure the CORS headers on their web server (probably unlikely).
  • Ask the Aladin developers to change how they test for CORS support from remote servers (I don't have an alternative in mind).
  • Ask the Aladin developers to provide an option that we can explicitly set to always proxy requests for a given image survey.
  • Move the HiPS images to Nimbus which we control and set the CORS headers (we may not have the disk space).

Otherwise, I think we just ignore it and move on. We seem to only get one or two errors, and thankfully not one for each image survey.

I guess it might not hurt asking if they could configure the CORS header, even though yes I agree it's very unlikely. Where would we direct that, do you know?

It's just a bit frustrating as while the HiPS load, they load poorly with the option to change the colour scheme missing.

But you're right about Nimbus, we don't want to be hosting them there.

So I'm not sure what the 'solution' would be for this apart from users installing the CORS extension to their browser!

@marxide
Copy link
Contributor

marxide commented Sep 4, 2020

Probably Vince McIntyre at CASS. Might be better to ask Emil first.

I don't think we need to do anything. The error is just the browser logging that a request failed, which Aladin expected and handles. It just doesn't suppress the error, I guess because there isn't a way to do so. The error is transparent to the user unless they're looking at the Javascript console.

srggrs
srggrs previously approved these changes Sep 4, 2020
Pipeline Backlog automation moved this from In progress to Reviewer approved Sep 4, 2020
@ajstewart
Copy link
Contributor Author

Probably Vince McIntyre at CASS. Might be better to ask Emil first.

I don't think we need to do anything. The error is just the browser logging that a request failed, which Aladin expected and handles. It just doesn't suppress the error, I guess because there isn't a way to do so. The error is transparent to the user unless they're looking at the Javascript console.

But the user does see the error indirectly by having a non-fully functional HiPS to look at.

But I agree there's nothing we can do about it.

@marxide
Copy link
Contributor

marxide commented Sep 5, 2020

Sorry, I missed that. You're right, I just spotted this:

Aladin Lite will be able to modify the color map if and only if the HTTP server hosting the image HiPS supports CORS (Cross-Origin Resource Sharing)
https://aladin.u-strasbg.fr/AladinLite/doc/API/examples/color-map/

Pretty frustrating that it tells us that on an "examples" page rather than in the API docs!

@ajstewart
Copy link
Contributor Author

I've asked Vince about this.

Pipeline Backlog automation moved this from Reviewer approved to Review in progress Sep 17, 2020
@ajstewart
Copy link
Contributor Author

@srggrs @marxide this can now be merged, all the CORS issues have been fixed at the ATNF end.

I've also added the VAST Stokes V HiPS that Emil has produced.

Pipeline Backlog automation moved this from Review in progress to Reviewer approved Sep 17, 2020
@srggrs
Copy link
Contributor

srggrs commented Sep 17, 2020

We can incorporate this in the first release

@ajstewart ajstewart merged commit 55d03a8 into master Sep 17, 2020
Pipeline Backlog automation moved this from Reviewer approved to Done Sep 17, 2020
@ajstewart ajstewart deleted the update-aladin-surveys branch September 17, 2020 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request UI User Interface
Projects
Development

Successfully merging this pull request may close these issues.

Update Aladin surveys with RACS and VAST
3 participants