Skip to content

[GPU] Adds support for multiple GPUs (and fixes their display order) #71

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
Merged
merged 6 commits into from
May 17, 2020
Merged

[GPU] Adds support for multiple GPUs (and fixes their display order) #71

merged 6 commits into from
May 17, 2020

Conversation

HorlogeSkynet
Copy link
Owner

@HorlogeSkynet HorlogeSkynet commented May 9, 2020 •
edited
Loading

Closes #61.

Description

GPUs were not following 3D > VGA > Display order (poor implementation), and only one was displayed.
This should be fixed now.

Reason and / or context

See #61.

How has this been tested ?

Test cases.

Types of changes :

  • Bug fix (non-breaking change which fixes an issue)
  • Typo / style fix (non-breaking change which improves readability)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist :

  • [IF NEEDED] I have updated the test cases (which pass) accordingly ;
  • My changes looks good ;
  • I agree that my code may be modified in the future ;
  • My code follows the code style of this project (PEP8).

@HorlogeSkynet HorlogeSkynet added bug 🐛 A real glitch has been found enhancement ⬆️ Implements a new feature, fixes or improves existing ones labels May 9, 2020
@HorlogeSkynet HorlogeSkynet added this to the v4.7.2 milestone May 9, 2020
@HorlogeSkynet HorlogeSkynet requested a review from ingrinder May 9, 2020 08:22
@HorlogeSkynet HorlogeSkynet self-assigned this May 9, 2020
@HorlogeSkynet HorlogeSkynet mentioned this pull request May 9, 2020
Closed
Samuel FORESTIER and others added 2 commits May 10, 2020 08:56
ingrinder
ingrinder reviewed May 10, 2020
View reviewed changes
Copy link
Collaborator

@ingrinder ingrinder left a comment

Choose a reason for hiding this comment

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

I've made a few tweaks to this PR; let me know what you think 👍.

Samuel FORESTIER added 3 commits May 11, 2020 10:29
[OUTPUT] Makes our append method prototype similar to list.append
234a3b6 
+ Automatically sets `Not detected` as `value` when "falsy" (that will DRY entries logic in the future).

Rationale: This would _drastically_ simplify its mocking across our test modules.
[GPU] Enables oneline by default for backward-compatibility purposes
b2ea1e8
[GPU] Globally simplifies GPU and adds test cases for GPU.output
fb74d8e
HorlogeSkynet pushed a commit that referenced this pull request May 11, 2020
[OUTPUT] [JSON] Prefers format entries values as list for the future
9a62470 
> Rationale: When #71 will be merged, issues due to keys duplication may occur.
>            This patch sets output values as a list anyway, sparing us an API breakage in the future.
> Note: A `Colors.remove_colors` utility method has been added for DRY purposes.
@HorlogeSkynet HorlogeSkynet requested a review from ingrinder May 11, 2020 09:28
@HorlogeSkynet
Copy link
Owner Author

HorlogeSkynet commented May 11, 2020 •
edited
Loading

So did I ! Entry.output became a problem when overridden by child classes.
I've tried a work-around, tell me what you think about that !

HorlogeSkynet
HorlogeSkynet commented May 11, 2020
View reviewed changes
@HorlogeSkynet HorlogeSkynet mentioned this pull request May 12, 2020
Merged
5 tasks
HorlogeSkynet pushed a commit that referenced this pull request May 12, 2020
[OUTPUT] [JSON] Prefers format entries values as list for the future
6d23de9 
> Rationale: When #71 will be merged, issues due to keys duplication may occur.
>            This patch sets output values as a list anyway, sparing us an API breakage in the future.
> Note: A `Colors.remove_colors` utility method has been added for DRY purposes.
ingrinder
ingrinder approved these changes May 17, 2020
View reviewed changes
Copy link
Collaborator

@ingrinder ingrinder left a comment

Choose a reason for hiding this comment

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

Nice! This all looks good to me now. Once #70 is merged, the falsy value to not-detected string should integrate quite nicely and simplify our Entry.output method overrides (I thought about adding a commit on this PR to clean all those entries up to set None values but then we'd just have problems merging #70 😦)

Anyway, this is a nice little PR to merge! 😆

@ingrinder ingrinder merged commit 930f4cf into master May 17, 2020
@HorlogeSkynet HorlogeSkynet deleted the feature_fix/multiple_gpus branch May 17, 2020 06:36
@HorlogeSkynet
Copy link
Owner Author

Actually, I wanted to merge #70 before this one, and then rebase those commits on top of master. But no problem, we'll be simply doing it the other way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ingrinder ingrinder ingrinder approved these changes

Assignees

@HorlogeSkynet HorlogeSkynet

Labels
bug 🐛 A real glitch has been found enhancement ⬆️ Implements a new feature, fixes or improves existing ones
Projects
Archey4 - Core
Status: DONE
Milestone
v4.7.2
Development

Successfully merging this pull request may close these issues.

[FEATURE] GPU support when multiple GPUs are present
2 participants
@HorlogeSkynet @ingrinder