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.

Sign up for GitHub

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

Jump to bottom

Fix SSL errors after forking process #1353

Merged
merged 5 commits into from Jun 14, 2023
Merged

Fix SSL errors after forking process #1353

merged 5 commits into from Jun 14, 2023

Conversation

aniezurawski
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Jun 14, 2023 •
edited

Codecov Report

Patch coverage: 50.00% and project coverage change: -0.09 ⚠️

Comparison is base (9c2de8f) 79.81% compared to head (ab0b851) 79.73%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1353      +/-   ##
==========================================
- Coverage   79.81%   79.73%   -0.09%     
==========================================
  Files         277      277              
  Lines       13592    13602      +10     
==========================================
- Hits        10849    10845       -4     
- Misses       2743     2757      +14
Flag Coverage Δ
py3.7 73.13% <50.00%> (-6.23%) ⬇️
py3.9 ?
ubuntu 73.13% <50.00%> (-6.45%) ⬇️
unit 73.13% <50.00%> (-0.71%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/neptune/common/backends/utils.py 55.55% <33.33%> (-2.34%) ⬇️
.../neptune/metadata_containers/metadata_container.py 88.36% <50.00%> (-2.10%) ⬇️
src/neptune/common/utils.py 83.33% <66.66%> (+1.85%) ⬆️
.../operation_processors/async_operation_processor.py 94.36% <100.00%> (+0.90%) ⬆️

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@aniezurawski aniezurawski force-pushed the waj/pg-ssl branch 4 times, most recently from 69ff96f to 7e15539 Compare June 14, 2023 09:28
Raalsky
Raalsky reviewed Jun 14, 2023
View reviewed changes
src/neptune/metadata_containers/metadata_container.py Outdated Show resolved Hide resolved
src/neptune/metadata_containers/metadata_container.py Show resolved Hide resolved
src/neptune/internal/operation_processors/async_operation_processor.py Outdated
@staticmethod
def _init_data_path(container_id: UniqueId, container_type: ContainerType):
now = datetime.now()
container_dir = f"{NEPTUNE_DATA_DIRECTORY}/{ASYNC_DIRECTORY}/{container_type.create_dir_name(container_id)}"
data_path = f"{container_dir}/exec-{now.timestamp()}-{now.strftime('%Y-%m-%d_%H.%M.%S.%f')}"
data_path = f"{container_dir}/exec-{os.getpid()}-{now.timestamp()}-{now.strftime('%Y-%m-%d_%H.%M.%S.%f')}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move the process id to the end. Currently, it breaks "sorting" capabilities :D.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

src/neptune/internal/operation_processors/async_operation_processor.py Outdated Show resolved Hide resolved
@PatrykGala
Copy link
Contributor

No need to register at fork in HostedNeptuneBackendApiClient and HostedAlphaLeaderboardApiClient.

  if sys.version_info >= (3, 7):
            try:
                os.register_at_fork(after_in_child=self._handle_fork_in_child)
            except AttributeError:
                pass

    def _handle_fork_in_child(self):
        self.backend_swagger_client = NoopObject()

@aniezurawski
Copy link
Contributor Author

aniezurawski commented Jun 14, 2023 •
edited

No need to register at fork in HostedNeptuneBackendApiClient and HostedAlphaLeaderboardApiClient.

@PatrykGala it's a legacy API. I don't want to touch its behaviour without a good reason.

I think it does not even use async processing and Run objects.

Raalsky
Raalsky reviewed Jun 14, 2023
View reviewed changes
src/neptune/metadata_containers/metadata_container.py Show resolved Hide resolved
src/neptune/metadata_containers/metadata_container.py
@@ -143,6 +148,37 @@ def __init__(

self._startup(debug_mode=mode == Mode.DEBUG)

os.register_at_fork(after_in_child=self._handle_fork_in_child, after_in_parent=self._handle_fork_in_parent)

"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this comment to reset_internal_ssl_state as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@aniezurawski aniezurawski merged commit 4d57e74 into master Jun 14, 2023
3 checks passed
@aniezurawski aniezurawski deleted the waj/pg-ssl branch June 14, 2023 11:35
aniezurawski added a commit that referenced this pull request Jun 14, 2023
@aniezurawski
Revert "Fix SSL errors after forking process (#1353)"
42db736 
This reverts commit 4d57e74.
aniezurawski added a commit that referenced this pull request Jun 14, 2023
@aniezurawski
Revert "Fix SSL errors after forking process (#1353)" (#1356)
f07bd9b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Raalsky Raalsky Raalsky approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@aniezurawski @PatrykGala @Raalsky