Skip to content

Conversation

@ch-iv
Copy link
Contributor

@ch-iv ch-iv commented Feb 24, 2025

This PR changes the resource limit on the number of extant processes (resource.RLIMIT_NPROC) to apply to each individual worker rather than to the entire docker user.

Refactors the set_rlimits_before_test function to remove the nproc adjustment, because it became obsolete due to earlier changes.

Adds a small change to how the rlimit value is validated. Previously the soft limit would be set to min(soft, hard), however that produced an incorrect result when hard = resource.RLIMIT_INFINITY (-1).

Adds tests for get_resource_settings, get_setrlimit_lines, and validate_rlimit.

@ch-iv
Copy link
Contributor Author

ch-iv commented Mar 1, 2025

@david-yz-liu Requesting review.

Copy link
Contributor

@david-yz-liu david-yz-liu left a comment

Choose a reason for hiding this comment

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

@ch-iv great work. I left a few inline comments; also please update the changelog.

@ch-iv ch-iv requested a review from david-yz-liu March 8, 2025 15:22


class MockTester(Tester):
def __init__(
Copy link
Contributor

Choose a reason for hiding this comment

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

Please omit the initializer entirely (as it isn't doing anything other than forwarding the arguments)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you please clarify how you want me to do this, because I've tried a few things, but none of them worked.

I've tried:

class MockTester(Tester):
    def run(self) -> None:
        pass

In the above code I get TypeError: Can't instantiate abstract class MockTester without an implementation for abstract method '__init__'.

I've also tried:

class MockTester(Tester):
    def __init__(
        self,
        specs: TestSpecs,
        test_class: Type[Test] | None = Test,
        resource_settings: list[tuple[int, tuple[int, int]]] | None = None,
    ) -> None:
        pass

    def run(self) -> None:
        pass

Which raises AttributeError: 'MockTester' object has no attribute 'resource_settings'.

Then I asked chat, which suggested to do:

class MockTester(Tester):
    __init__ = Tester.__init__

    def run(self) -> None:
        pass

But I still get TypeError: Can't instantiate abstract class MockTester without an implementation for abstract method '__init__'

I think this is because the Tester.__init__ method is defined as abstract, so it has to be implemented in the MockTester class. However, Tester.__init__ also does some setup, so all classes inhering from Tester must either call Tester.__init__ or reimplement that setup logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ch-iv you're right about this, I had missed the @abstractmethod decorator on the initializer. I'm good to leave this as-is!

@ch-iv ch-iv requested a review from david-yz-liu March 9, 2025 15:39
Copy link
Contributor

@david-yz-liu david-yz-liu left a comment

Choose a reason for hiding this comment

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

Nice work, @ch-iv!



class MockTester(Tester):
def __init__(
Copy link
Contributor

Choose a reason for hiding this comment

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

@ch-iv you're right about this, I had missed the @abstractmethod decorator on the initializer. I'm good to leave this as-is!

@david-yz-liu david-yz-liu merged commit e433e85 into MarkUsProject:master Mar 9, 2025
7 checks passed
@david-yz-liu david-yz-liu added this to the v2.6.1 milestone Mar 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants