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

Initial overhaul of Profile mode #200

Merged
merged 9 commits into from
Dec 1, 2023
Merged

Initial overhaul of Profile mode #200

merged 9 commits into from
Dec 1, 2023

Conversation

coleramos425
Copy link
Collaborator

My initial PR includes a redesign of our "Profile mode" code. From a UX perspective, nothing should be different. All your usual profile commands will run just as before.

Under the hood, I've rearranged the /src/ directory to reflect some files that have been deleted, renamed, or otherwise integrated into one of our classes as helper methods.

To start, I've just tested these changes on a Mi200 + rocprofv1 environment. My plan is to first agree on structuring for this combination, then use that to inform the others.

At a high level, the new design is shown in the "New structure" section below. Any feedback would be highly appreciated - I'm most curious if any improvements can be made to my encapsulation, overall organization and/or logging.

New structure

image

image

image

Signed-off-by: colramos-amd <colramos@amd.com>
@coleramos425 coleramos425 requested a review from koomie October 31, 2023 20:36
Copy link

github-actions bot commented Oct 31, 2023

Coverage

Coverage Report
FileStmtsMissCoverMissing
argparser.py82396%136, 145, 154
config.py30100% 
omniperf14378%40, 44, 47
omniperf_base.py1526259%85–93, 95–96, 110–113, 121–123, 125, 139, 141, 144–145, 147, 154–155, 157–158, 162–164, 166–167, 186–187, 198, 206–211, 213–214, 221–224, 226, 230, 232, 241–246, 248, 254–256, 258
roofline.py1321320%25–35, 37, 39–41, 43–48, 50–51, 53–54, 57–60, 62–66, 68–70, 72–75, 78–83, 86–87, 89, 93–96, 98–100, 102, 104–107, 110–112, 114–121, 132–133, 138–142, 147–148, 150, 153–154, 172–173, 190–191, 193, 195, 214, 216, 226, 236, 248, 254–255, 257, 259–260, 272–274, 277, 283, 289, 297–298, 300–303, 306, 309, 312, 316, 325, 334, 337–338, 340–341, 345–347, 349–350, 353–355, 357
omniperf_profile
   profiler_base.py714930%42–45, 55, 58–59, 62–66, 68, 71–74, 80, 83–90, 92–94, 96, 99, 101–102, 112–113, 115, 118–119, 129–130, 132–134, 136, 138–139, 142, 148–149
   profiler_rocprof_v1.py1209620%48–50, 56–59, 61, 67–68, 70, 72, 74, 80–81, 84–86, 88–90, 93–95, 98–103, 105, 108, 111, 119–124, 126–127, 129–137, 141–142, 144–145, 148, 151, 160–161, 164–169, 174, 176–179, 183, 215, 231–235, 237–243, 245–246, 248–251, 253, 255–259, 261
omniperf_soc
   soc_base.py17913126%43, 67–69, 72–73, 75, 77, 79–81, 84–87, 89–95, 97, 101, 103–104, 108–109, 123, 129, 135, 138–139, 154–155, 158–159, 161, 163–165, 168–170, 173, 176–177, 182–187, 190–191, 193, 202–204, 207–209, 224–225, 227, 229–230, 232, 234–239, 241, 244–245, 247–248, 250, 252–253, 257–258, 260–261, 265, 268, 270–271, 274, 276, 278–280, 286, 293, 295–296, 302, 305–307, 309, 311–312, 314, 317–322, 325–326, 328, 330–331, 335, 338–340, 342–344, 346–350, 352
   soc_gfx908.py23291%69, 75
utils
   __init__.py00100% 
   csv_processor.py14112213%47, 50–54, 56, 58–61, 63, 65, 69–70, 73–74, 76, 81–82, 85–88, 90–91, 93–97, 99–100, 102–104, 107–108, 110, 112–119, 121–123, 125, 127, 130–133, 138, 140–142, 147–150, 152, 157–159, 161–162, 164, 167–169, 171, 173–176, 178–179, 181, 183–184, 186, 188–192, 194, 196, 198–199, 201, 210, 215, 226–231, 233–240, 251–254, 256–262
   gfx_perfmon_builder.py1891890%25, 32, 40–42, 46–48, 56, 70, 76–77, 79–80, 83–85, 87, 89, 92, 96–100, 102, 104–106, 108–110, 113, 120–121, 123–126, 128–131, 133–136, 139–142, 144–147, 152–154, 156–159, 164–165, 167–168, 170–174, 176–177, 179, 182–185, 187, 189, 191, 193, 195–201, 203, 205, 209–210, 213–214, 217, 219–220, 222–224, 226–227, 230, 233, 236, 239–240, 242–245, 248–252, 254–256, 258–259, 261, 265–266, 268–269, 272, 275–276, 279, 281–283, 286–288, 290, 292, 294, 296–297, 299–300, 302, 304, 306–309, 312–314, 316–318, 322, 325, 327, 329, 331–332, 335–338, 340, 342–345, 348, 350–353, 363, 368–371, 380, 383–385, 388, 391, 393, 396
   remove_workload.py27270%27–29, 33, 42–44, 46–50, 52, 54–55, 58–59, 61, 63, 71, 83, 86, 90–93, 95
   resources.py770%25–26, 29–31, 34–35
   roofline_calc.py2102100%25, 27–28, 34, 36, 38–39, 41–43, 45, 47, 53–56, 58–68, 70–71, 74–75, 83–89, 91, 97, 101, 103–104, 106, 108–110, 112–113, 116, 118–121, 123–124, 126, 130–131, 133–135, 138–139, 141–142, 145–147, 149–151, 157, 159–161, 163–167, 170, 173–175, 177–181, 183, 190, 193, 195–196, 198, 214, 216–218, 220, 225–226, 228, 230–232, 265–270, 293–296, 298–307, 309–310, 315–318, 320–325, 327–328, 334–339, 345–348, 350–351, 353, 355–356, 375–376, 381, 399–400, 419, 437, 440–443, 445–447, 452, 457, 462, 467, 469, 471–472, 474–479, 481–482, 485, 487, 490–491, 495, 498–508, 510–511, 513–516, 524, 529, 533
   specs.py1331787%62, 97, 152–153, 191–193, 197, 199–201, 205–206, 230, 237, 241, 266
   utils.py19413530%61–63, 74–75, 80–86, 88, 111, 116–117, 120–121, 124, 137, 144–145, 148–150, 153–154, 157, 159–162, 165–166, 168, 171–172, 174, 178, 180, 183–185, 200, 213, 216–217, 219–220, 222–224, 226–227, 231–232, 235–241, 244–248, 250–252, 262, 271, 280–289, 292–294, 296–297, 299–300, 303–304, 306–309, 311–315, 317–320, 322–323, 329–330, 332, 334–335, 337–338, 342–345, 356–359, 362–364, 369–370, 372–374, 376, 388–390, 392
TOTAL1677118529% 

Tests Skipped Failures Errors Time
49 0 💤 49 ❌ 0 🔥 14.719s ⏱️

@feizheng10
Copy link
Contributor

Few quick thoughts:

  • I won't call it "metric_configs" because from the design view the configs including both front-end panel/table style and back-end metrics definition/description unless we'd like to separate those in the future.
  • Don't see the def of roofline_obj. I don't see any reason it associates to gfx90a_soc(even though we only provide it for gfx90a) .

@coleramos425
Copy link
Collaborator Author

Don't see the def of roofline_obj. I don't see any reason it associates to gfx90a_soc(even though we only provide it for gfx90a)

Thanks for your feedback, @feizheng10. The idea is to, at a high level, put any SoC specific material in these SoC subclasses. Since Roofline (specifically MFMA) isn't available on all hardware types, I decided to make this a property of gfx90a.

Additionally, our roofline calculation may change in future hardware. Instantiating the roofline object at the SoC level gives us the ability to override any parts of the roofline calc from within these SoC subclasses. Let me know if this makes sense

@feizheng10
Copy link
Contributor

feizheng10 commented Nov 3, 2023

Don't see the def of roofline_obj. I don't see any reason it associates to gfx90a_soc(even though we only provide it for gfx90a)

Thanks for your feedback, @feizheng10. The idea is to, at a high level, put any SoC specific material in these SoC subclasses. Since Roofline (specifically MFMA) isn't available on all hardware types, I decided to make this a property of gfx90a.

Additionally, our roofline calculation may change in future hardware. Instantiating the roofline object at the SoC level gives us the ability to override any parts of the roofline calc from within these SoC subclasses. Let me know if this makes sense

I mean, technically, roofline is nothing special other than other ip blocks which should be configed int the similar way. I'm planing to improve that in my working branch.

@feizheng10
Copy link
Contributor

BTW, prefix "__" for members in class seems not recommended in PEP8.

@koomie
Copy link
Collaborator

koomie commented Nov 6, 2023

BTW, prefix "__" for members in class seems not recommended in PEP8.

I'm a fan of demarcating variables that we intend to treat as private class vars (since Python doesn't have private vars directly). It doesn't have to be underscores, but I think we should pick a convention for items we are sharing across member functions and try to stick with it. btw, although it may not be mentioned in PEP8, this common convention is still mentioned in latest python docs, e.g.:

https://docs.python.org/3.11/tutorial/classes.html#private-variables

@koomie
Copy link
Collaborator

koomie commented Nov 6, 2023

BTW, prefix "__" for members in class seems not recommended in PEP8.

Also, in the section on designing for inheritance for PEP8 (https://peps.python.org/pep-0008/#designing-for-inheritance) there is still a reference to the use of leading underscore for non-public attributes (sort of in a backwards way). It says:

With this in mind, here are the Pythonic guidelines:

  • Public attributes should have no leading underscores.

So, while there is really no such thing as private attributes, the notion of demarcating those that are intended to be used as private for developers seems to remain.

Signed-off-by: colramos-amd <colramos@amd.com>
Signed-off-by: colramos-amd <colramos@amd.com>
Signed-off-by: colramos-amd <colramos@amd.com>
@coleramos425
Copy link
Collaborator Author

Update:
I've just added changes to the vcopy app included with Omniperf. It will now support multi-dispatch and multi-kernel requirements for our CI tests.

Note, there's a hidden option in vcopy to activate multi-kernel (to avoid confusing customer) it's called --multikernel

src/omniperf_base.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
src/argparser.py Outdated Show resolved Hide resolved
src/docs/profiling.md Outdated Show resolved Hide resolved
src/docs/profiling.md Outdated Show resolved Hide resolved
src/omniperf Outdated Show resolved Hide resolved
Signed-off-by: colramos-amd <colramos@amd.com>
…tion

Signed-off-by: colramos-amd <colramos@amd.com>
Signed-off-by: colramos-amd <colramos@amd.com>
Signed-off-by: colramos-amd <colramos@amd.com>
Copy link
Collaborator

@koomie koomie left a comment

Choose a reason for hiding this comment

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

Thanks for quick updates on pyfiglet and --kernel-verbose.

I'm in favor of going ahead and landing this.

@koomie koomie assigned koomie and unassigned koomie Dec 1, 2023
@koomie koomie added this to the v2.0.0 milestone Dec 1, 2023
@koomie koomie merged commit bd803a0 into 2.x Dec 1, 2023
2 of 3 checks passed
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.

3 participants