-
Notifications
You must be signed in to change notification settings - Fork 28
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
switch jenkins runs to use roman-serverless for crds #946
switch jenkins runs to use roman-serverless for crds #946
Conversation
00d0b9e
to
85b4f52
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #946 +/- ##
=======================================
Coverage 76.74% 76.74%
=======================================
Files 105 105
Lines 7013 7013
=======================================
Hits 5382 5382
Misses 1631 1631
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. |
All regression tests passed. The output of the
showing no error and that the |
Where is https://roman-serverless defined? |
I'm not sure if the crds docs describe this (beyond what's below). Internally, crds calls """Based on the environment, cache, and server, determine the default observatory.
1. CRDS_OBSERVATORY env var
2. CRDS_SERVER_URL env var
3. Observatory(Server default context)
4. jwst
""" As This is similar to what is done with jwst when |
I'm seeing
So I'm not sure where this is being cached and when/who will update these. I'm beginning to think we should revert this to the earlier version without the "crds list" command for now. I'm not sure that setting the variable above and resetting it with "crds list" really makes sense. Then we can fix this in GH actions? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As described, the code change is meaningless.
Reading through the comments and noting that the CI is passing, is there something still up? If so, it is not clear in the commentary what the issues still are.
@@ -53,7 +53,7 @@ def pip_install_args = "--index-url ${pip_index} --progress-bar=off" | |||
env_vars = [ | |||
"TEST_BIGDATA=https://bytesalad.stsci.edu/artifactory", | |||
"CRDS_CONTEXT=roman_0051.pmap", | |||
"CRDS_SERVER_URL=https://serverless", | |||
"CRDS_SERVER_URL=https://roman-serverless", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change, from the crds perspective, is a NOOP. The only condition to forcing serverless mode is having "serverless" in the url, as it already was pre-change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking a look. Yes this is still an issue. Every run without this change is silently failing the crds context lookup step with the following error (which @nden pointed out):
++ crds list --contexts roman_0051.pmap --mappings
++ grep pmap
CRDS - ERROR - (FATAL) CRDS server connection and cache load FAILED. Cannot continue.
See https://hst-crds.stsci.edu/docs/cmdline_bestrefs/ or https://jwst-crds.stsci.edu/docs/cmdline_bestrefs/
for more information on configuring CRDS, particularly CRDS_PATH and CRDS_SERVER_URL. : [Errno 2] No such file or directory: '/grp/crds/roman/test/config/jwst/server_config'
+ echo 'CRDS_CONTEXT = '
CRDS_CONTEXT =
As @ddavis-stsci commented it might be cleaner to just remove crds list
entirely (I don't know why this was added in the first place).
I understand that both serverless
and roman-serverless
are equivalent for forcing serverless mode. However this change does effect what observatory is inferred from the CRDS_SERVER_URL
as mentioned in the comment above and as seen in the jenkins runs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, my mistake. I had presumed that the "serverless" indicator precluded any other server-based checking, but it does not. If the observatory is unknown, crds still attempts to pull the name from the CRDS_SERVER_URL. So, yes, this change will enforce the use of the roman side of crds.
So, this PR is approved.
However, curiosity question: At this point in the CI, how has crds
been installed? Directly or presumed installed with romancal
?
For jwst, I believe the crds list
command was there just to log what the state of the crds system is during CI. Leaving it in or out is up to the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crds is installed just prior to the call to crds list
. It's installed as a dependency of romancal
:
Line 89 in c3a7025
"pip install -e .[test]", |
85b4f52
to
773c310
Compare
773c310
to
a9949c2
Compare
ddtrace ci failures are unrelated and caused by incompatibility between pytest 8 and ddtrace: |
Jonathan's approval is good enough for me. I think we should merge. |
Jenkins runs show errors in the logs for the
crds list
step:from: https://plwishmaster.stsci.edu:8081/blue/organizations/jenkins/RT%2Fromancal/detail/romancal/1092/pipeline
This appears to be due to crds defaulting to
jwst
when an observatory is not defined.This PR updates the
CRDS_SERVER
environment variable to allow crds to know that this isroman
.Regression tests running at:
https://plwishmaster.stsci.edu:8081/job/RT/job/Roman-Developers-Pull-Requests/432/
Checklist
CHANGES.rst
under the corresponding subsection