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

Update Z2JH to 4.0.0 #1884

Merged
merged 8 commits into from
Nov 10, 2024
Merged

Update Z2JH to 4.0.0 #1884

merged 8 commits into from
Nov 10, 2024

Conversation

manics
Copy link
Member

@manics manics commented Nov 7, 2024

@consideRatio
Copy link
Member

consideRatio commented Nov 7, 2024

[C 2024-11-07 13:26:52.175 JupyterHub application:120] Bad config encountered during initialization: The 'authenticator_class' trait of <jupyterhub.app.JupyterHub object at 0x7fe39a37cfb0> instance must be a type, but 'nullauthenticator.NullAuthenticator' could not be imported

I think its no longer part of z2jh hub image, because its now available in jupyterhub itself by specifying null. I figure this may be a change we should have had in the changelog if we didnt already as breaking

Edit: https://github.com/jupyterhub/zero-to-jupyterhub-k8s/pull/3523/files

`ERROR binderhub/tests/test_build.py - ValueError: mark.asyncio accepts only a keyword argument 'scope'.`
@@ -21,7 +21,7 @@

# We have optimized this slow test, for more information, see the README of
# https://github.com/binderhub-ci-repos/minimal-dockerfile.
@pytest.mark.asyncio(timeout=900)
@pytest.mark.asyncio
Copy link
Member Author

Choose a reason for hiding this comment

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

According to pytest-dev/pytest-asyncio#215 the timeout argument was only a proposal.
This means the argument would've been silently ignored until additional error checking was added in pytest-dev/pytest-asyncio#886

@manics manics added dependencies Pull requests that update a dependency file breaking labels Nov 7, 2024
@manics
Copy link
Member Author

manics commented Nov 8, 2024

The test failure is due to #1886
(though we're only showing a max of two failures so there may be more)

@consideRatio consideRatio reopened this Nov 9, 2024
@consideRatio
Copy link
Member

Ckan resolved, next error is now in binderhub/tests/test_eventlog.py::test_emit_event

@manics
Copy link
Member Author

manics commented Nov 9, 2024

ba490a7
deletes the taskName attribute.
This is a new logging attribute in Python 3.13:
https://docs.python.org/3.13/library/logging.html#logrecord-attributes

I don't understand how the EventLog works though. It only removes the message attribute

def _skip_message(record, **kwargs):
"""
Remove 'message' from log record.
It is always emitted with 'null', and we do not want it,
since we are always emitting events only
"""
del record["message"]
return json.dumps(record, **kwargs)
class EventLog(Configurable):
"""
Send structured events to a logging sink
"""
handlers_maker = Callable(
None,
config=True,
allow_none=True,
help="""
Callable that returns a list of logging.Handler instances to send events to.
When set to None (the default), events are discarded.
""",
)
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.log = logging.getLogger(__name__)
# We don't want events to show up in the default logs
self.log.propagate = False
self.log.setLevel(logging.INFO)
if self.handlers_maker:
self.handlers = self.handlers_maker(self)
formatter = jsonlogger.JsonFormatter(json_serializer=_skip_message)
for handler in self.handlers:
handler.setFormatter(formatter)
self.log.addHandler(handler)
self.schemas = {}
def register_schema(self, schema):
"""
Register a given JSON Schema with this event emitter
'version' and '$id' are required fields.
"""
# Check if our schema itself is valid
# This throws an exception if it isn't valid
jsonschema.validators.validator_for(schema).check_schema(schema)
# Check that the properties we require are present
required_schema_fields = {"$id", "version"}
for rsf in required_schema_fields:
if rsf not in schema:
raise ValueError(f"{rsf} is required in schema specification")
# Make sure reserved, auto-added fields are not in schema
reserved_fields = {"timestamp", "schema", "version"}
for rf in reserved_fields:
if rf in schema["properties"]:
raise ValueError(
f"{rf} field is reserved by event emitter & can not be explicitly set in schema"
)
self.schemas[(schema["$id"], schema["version"])] = schema
def emit(self, schema_name, version, event):
"""
Emit event with given schema / version in a capsule.
"""
if not self.handlers_maker:
# If we don't have a handler setup, ignore everything
return
if (schema_name, version) not in self.schemas:
raise ValueError(f"Schema {schema_name} version {version} not registered")
schema = self.schemas[(schema_name, version)]
jsonschema.validate(event, schema)
capsule = {
"timestamp": datetime.utcnow().isoformat() + "Z",
"schema": schema_name,
"version": version,
}
capsule.update(event)
self.log.info(capsule)

but https://docs.python.org/3.13/library/logging.html#logrecord-attributes lists many more attributes.

@consideRatio
Copy link
Member

consideRatio commented Nov 10, 2024

About logging change: makes sense to me given what i get about the logging, which isnt much, but i dont see how kt would be breaking or similar to do this change now that python 3.12 is used and taskname taskname stuff got introduced (it was in 3.12, not 3.13)

@consideRatio consideRatio merged commit a93a01c into jupyterhub:main Nov 10, 2024
14 checks passed
@consideRatio
Copy link
Member

Thank you @manics!!!

consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request Nov 10, 2024
@manics manics deleted the update-z2jh branch November 10, 2024 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants